chore: Improve errors.py (#100)

* Let ServerError take an `extra_message` argument

This will fix an error we have in our usage of this error.

* Use f-strings throughout

* Use enum value rather than bare int for error code

* Update CHANGELOG

* Fix bad docs conf value

* Add some unit tests for ApiError

* Toopher -> DDA in comment

* Use "additional_info" instead of "extra_message" for consistency

* Expand testing

* Make message formatting standard

* Blacken

* Minor change in error message

* Improve error_code property a bit

* Test ClientErrorResponseWrapper and make a small tweak

* Add more tests

* Blacken

* Update CHANGELOG

* Bump coverage check to 91%

* Only log additional_info

Requires running tests with logging on

Author: bgrant

CHANGELOG.md

@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+# [unreleased] - XXXX-XX-XX
+### Added
+- [PR 100](https://github.com/salesforce/django-declarative-apis/pull/100) Improve `errors.py`
+
 # [0.23.1] - 2022-05-17
 ### Fixed
 - [PR 98](https://github.com/salesforce/django-declarative-apis/pull/98) Fix GitHub publish action

django_declarative_apis/machinery/errors.py

@@ -12,7 +12,8 @@
 
 import http.client
 
-# Start Toopher-specific codes at 600 to avoid conflict/confusion with HTTP status codes
+
+# Start DDA-specific codes at 600 to avoid conflict/confusion with HTTP status codes
 import sys
 
 logger = logging.getLogger(__name__)
@@ -75,10 +76,8 @@ def error_code(self):
             return self.__dict__["error_code"]
         except KeyError:
             raise AttributeError(
-                "'{}' object has no attribute '{}'".format(
-                    ApiError.__name__, "error_code"
-                )
-            )
+                f"'{ApiError.__name__}' object has no attribute 'error_code'"
+            ) from None  # suppress reporting the KeyError
 
     @error_code.setter
     def error_code(self, value):
@@ -113,7 +112,7 @@ def __init__(self, additional_info=None):
             http.client.responses.get(http.client.NOT_FOUND),
         )
         if additional_info:
-            error_message += " : " + additional_info
+            error_message += f": {additional_info}"
         super().__init__(
             error_code, error_message, http_status_code=http.client.NOT_FOUND
         )
@@ -123,7 +122,7 @@ class ClientErrorForbidden(ClientError):
     def __init__(self, additional_info=None, **kwargs):
         error_code, error_message = FORBIDDEN
         if additional_info:
-            error_message += ": %s" % additional_info
+            error_message += f": {additional_info}"
         super().__init__(
             error_code, error_message, http_status_code=http.client.FORBIDDEN, **kwargs
         )
@@ -147,21 +146,25 @@ class ClientErrorExternalServiceFailure(ClientError):
     def __init__(self, additional_info=None):
         error_code, error_message = EXTERNAL_REQUEST_FAILURE
         if additional_info:
-            error_message += ": {0}".format(additional_info)
+            error_message += f": {additional_info}"
         super().__init__(error_code, error_message)
 
 
 class ClientErrorRequestThrottled(ClientError):
     def __init__(self):
         error_code, error_message = REQUEST_THROTTLED
-        super().__init__(error_code, error_message, http_status_code=429)
+        super().__init__(
+            error_code,
+            error_message,
+            http_status_code=http.HTTPStatus.TOO_MANY_REQUESTS,
+        )
 
 
 class ClientErrorTimedOut(ClientError):
     def __init__(self, additional_info=None):
         error_code, error_message = TIMED_OUT
         if additional_info:
-            error_message += ": %s" % additional_info
+            error_message += f": {additional_info}"
         super().__init__(
             error_code, error_message, http_status_code=http.client.REQUEST_TIMEOUT
         )
@@ -170,7 +173,7 @@ def __init__(self, additional_info=None):
 class ClientErrorResponseWrapper(ClientError):
     def __init__(self, response):
         error_code = response.status_code
-        error_message = response.content
+        error_message = response.content.decode()
         status_code = response.status_code
         super().__init__(error_code, error_message, http_status_code=status_code)
 
@@ -179,43 +182,45 @@ class ClientErrorExtraFields(ClientError):
     def __init__(self, extra_fields=None):
         error_code, error_message = EXTRA_FIELDS
         if extra_fields:
-            error_message += ": %s" % ", ".join(extra_fields)
+            error_message += f": {', '.join(extra_fields)}"
         super().__init__(error_code, error_message)
 
 
 class ClientErrorReadOnlyFields(ClientError):
     def __init__(self, read_only_fields=None):
         error_code, error_message = READ_ONLY_FIELDS
         if read_only_fields:
-            error_message += ": %s" % ", ".join(read_only_fields)
+            error_message += f": {', '.join(read_only_fields)}"
         super().__init__(error_code, error_message)
 
 
 class ClientErrorMissingFields(ClientError):
     def __init__(self, missing_fields=None, extra_message=None):
         error_code, error_message = MISSING_FIELDS
         if missing_fields:
-            error_message += ": %s" % ", ".join(missing_fields)
+            error_message += f": {', '.join(missing_fields)}"
         if extra_message:
-            error_message += ": {0}".format(extra_message)
+            error_message += f": {extra_message}"
         super().__init__(error_code, error_message)
 
 
 class ClientErrorInvalidFieldValues(ClientError):
     def __init__(self, invalid_fields=None, extra_message=None):
         error_code, error_message = INVALID_FIELD_VALUES
         if invalid_fields:
-            error_message += ": %s" % ", ".join(invalid_fields)
+            error_message += f": {', '.join(invalid_fields)}"
         if extra_message:
-            error_message += ": {0}".format(extra_message)
+            error_message += f": {extra_message}"
         super().__init__(error_code, error_message)
 
 
 class ServerError(ApiError):
-    def __init__(self):
+    def __init__(self, additional_info=None):
         error_code, error_message = LOGGED_SERVER_ERROR
-        error_message = error_message.format(uuid.uuid4())
-        logger.exception(error_message)
+        logged_error = error_message = error_message.format(uuid.uuid4())
+        if additional_info:
+            logged_error += f": {additional_info}"
+        logger.exception(logged_error)
 
         self._cause = None
 

docs/source/conf.py

@@ -84,7 +84,7 @@
 #
 # This is also used if you do content translation via gettext catalogs.
 # Usually you set "language" from the command line for these cases.
-language = None
+language = 'en'
 
 # There are two options for replacing |today|: either, you set today to some
 # non-false value, then it is used:

pyproject.toml

@@ -23,5 +23,5 @@ exclude = '''
 omit = ["*/tests/*", "*/management/*", "*/migrations/*"]
 
 [tool.coverage.report]
-fail_under = 88
+fail_under = 91
 exclude_lines = ["raise NotImplementedError"]

tests/machinery/test_base.py

@@ -78,7 +78,7 @@ def resource(self):
             self.fail("This should have failed")
         except errors.ClientErrorNotFound as err:
             self.assertEqual(err.error_code, http.HTTPStatus.NOT_FOUND)
-            self.assertEqual(err.error_message, "Not Found : dict instance not found")
+            self.assertEqual(err.error_message, "Not Found: dict instance not found")
             self.assertEqual(err.status_code, http.HTTPStatus.NOT_FOUND)
             self.assertFalse(err.save_changes)
             self.assertEqual(err.extra_fields, {})

tests/machinery/test_errors.py

@@ -0,0 +1,115 @@
+import django.test
+from django import http
+
+from django_declarative_apis.machinery import errors
+
+
+class ErrorTestCast(django.test.TestCase):
+    def test_apierror_tuple(self):
+        test_code, test_message = test_tuple = errors.HTTPS_REQUIRED
+        err = errors.ApiError(error_tuple=test_tuple)
+        self.assertEqual(test_code, err.error_code)
+        self.assertEqual(test_message, err.error_message)
+        self.assertDictEqual(
+            err.as_dict(), dict(error_code=test_code, error_message=test_message)
+        )
+
+    def test_apierror_code_and_message(self):
+        test_code, test_message = 600, "test message"
+        err = errors.ApiError(code=test_code, message=test_message)
+        self.assertEqual(test_code, err.error_code)
+        self.assertEqual(test_message, err.error_message)
+        self.assertDictEqual(
+            dict(error_code=test_code, error_message=test_message), err.as_dict()
+        )
+
+    def test_apierror_bad_args(self):
+        try:
+            errors.ApiError()
+        except errors.ApiError:
+            errmsg = "ApiError requires arguments"
+            self.fail(errmsg)
+        except Exception:  # this is the expected result
+            pass
+        else:
+            self.fail("ApiError without arguments should raise an exception.")
+
+    def test_apierror_extra_args(self):
+        test_code, test_message = test_tuple = errors.AUTHORIZATION_FAILURE
+        err = errors.ApiError(error_tuple=test_tuple, foo="bar", baz="quux")
+        self.assertDictEqual(
+            dict(
+                error_code=test_code, error_message=test_message, foo="bar", baz="quux"
+            ),
+            err.as_dict(),
+        )
+
+    def test_apierror_deprecated_error_code(self):
+        test_err = (999, "This is a fake error code.")
+        try:
+            errors.DEPRECATED_ERROR_CODES[test_err[0]] = test_err[1]
+            with self.assertRaises(ValueError):
+                with self.assertWarns(DeprecationWarning):
+                    errors.ApiError(error_tuple=test_err)
+        finally:
+            del errors.DEPRECATED_ERROR_CODES[test_err[0]]
+
+    def test_apierror_error_code_attributeerror(self):
+        test_error_tuple = (999, "This is a fake error code.")
+        err = errors.ApiError(error_tuple=test_error_tuple)
+        del err.__dict__["error_code"]
+        with self.assertRaises(AttributeError):
+            err.error_code
+
+    def test_additional_info_in_error(self):
+        test_classes = [
+            errors.ClientErrorUnprocessableEntity,
+            errors.ClientErrorNotFound,
+            errors.ClientErrorForbidden,
+            errors.ClientErrorUnauthorized,
+            errors.ClientErrorExternalServiceFailure,
+            errors.ClientErrorTimedOut,
+        ]
+        test_message = "Test additional info."
+        for cls in test_classes:
+            with self.subTest(cls):
+                err = cls(additional_info=test_message)
+                self.assertIn(test_message, err.error_message)
+
+    def test_servererror(self):
+        test_additional_info = "Test additional info."
+        with self.assertLogs(logger="django_declarative_apis.machinery.errors") as logs:
+            err = errors.ServerError(additional_info=test_additional_info)
+        self.assertIn("Server Error", err.error_message)
+        self.assertNotIn(test_additional_info, err.error_message)
+        self.assertIn("Server Error", logs.output[0])
+        self.assertIn(test_additional_info, logs.output[0])
+
+    def test_clienterrorresponsewrapper(self):
+        message = "It's gone!"
+        response = http.HttpResponseGone(message)
+        err = errors.ClientErrorResponseWrapper(response)
+        self.assertEqual(response.status_code, err.error_code)
+        self.assertEqual(message, err.error_message)
+
+    def test_clienterrorextrafields(self):
+        err = errors.ClientErrorExtraFields(extra_fields=["foo", "bar", "baz"])
+        self.assertIn("foo", err.error_message)
+        self.assertIn("bar", err.error_message)
+        self.assertIn("baz", err.error_message)
+
+    def test_clienterrorreadonlyfields(self):
+        err = errors.ClientErrorReadOnlyFields(read_only_fields=["foo", "bar", "baz"])
+        self.assertIn("foo", err.error_message)
+        self.assertIn("bar", err.error_message)
+        self.assertIn("baz", err.error_message)
+
+    def test_clienterrormissingfields(self):
+        extra_message = "Test extra message."
+        err = errors.ClientErrorMissingFields(
+            missing_fields=["foo", "bar", "baz"], extra_message=extra_message
+        )
+        self.assertIn("foo", err.error_message)
+        self.assertIn("bar", err.error_message)
+        self.assertIn("baz", err.error_message)
+        self.assertIn(extra_message, err.error_message)

tests/settings.py

@@ -21,8 +21,6 @@
 
 ROOT_URLCONF = "tests.urls"
 
-TEST_RUNNER = "tests.testutils.NoLoggingTestRunner"
-
 MIDDLEWARE = []
 
 REQUIRE_HTTPS_FOR_OAUTH = False