JsonableError: Optionally carry error codes and structured data.

This provides the main infrastructure for fixing #5598.  From here,
it's a matter of on the one hand upgrading exception handlers -- the
many except-blocks in the codebase that look for JsonableError -- to
look beyond the string `msg` and pass on the machine-readable full
error information to their various downstream recipients, and on the
other hand adjusting places where we raise errors to take advantage
of this mechanism to give the errors structured details.

In an ideal future, I think all exception handlers that look (or
should look) for a JsonableError would use its contents in structured
form, never mentioning `msg`; but the majority of error sites might
continue to just instantiate JsonableError with a string message.  The
latter is the simplest thing to do, and probably most error types will
never have code looking for them specifically.

Because the new API refactors the `to_json_error_msg` method which was
designed for subclasses to override, update the 4 subclasses that did
so to take full advantage of the new API instead.

zerver/lib/exceptions.py

@@ -1,23 +1,143 @@
 from __future__ import absolute_import
-from typing import Text
+from enum import Enum
+from typing import Any, Dict, List, Optional, Text, Type
 
 from django.core.exceptions import PermissionDenied
 
+class AbstractEnum(Enum):
+    '''An enumeration whose members are used strictly for their names.'''
+
+    def __new__(cls):
+        # type: (Type[AbstractEnum]) -> AbstractEnum
+        obj = object.__new__(cls)
+        obj._value_ = len(cls.__members__) + 1  # type: ignore # typeshed enum missing Enum.__members__
+        return obj
+
+    # Override all the `Enum` methods that use `_value_`.
+
+    def __repr__(self):
+        # type: () -> str
+        return str(self)
+
+    def value(self):
+        # type: () -> None
+        assert False
+
+    def __reduce_ex__(self, proto):
+        # type: (int) -> None
+        assert False
+
+class ErrorCode(AbstractEnum):
+    BAD_REQUEST = ()  # Generic name, from the name of HTTP 400.
+    REQUEST_VARIABLE_MISSING = ()
+    REQUEST_VARIABLE_INVALID = ()
+    BAD_NARROW = ()
+    UNAUTHORIZED_PRINCIPAL = ()
+
 class JsonableError(Exception):
-    msg = None  # type: Text
+    '''A standardized error format we can turn into a nice JSON HTTP response.
+
+    This class can be invoked in several ways.
+
+     * Easiest, but completely machine-unreadable:
+
+         raise JsonableError(_("No such widget: {}").format(widget_name))
+
+       The message may be passed through to clients and shown to a user,
+       so translation is required.  Because the text will vary depending
+       on the user's language, it's not possible for code to distinguish
+       this error from others in a non-buggy way.
+
+     * Partially machine-readable, with an error code:
+
+         raise JsonableError(_("No such widget: {}").format(widget_name),
+                             ErrorCode.NO_SUCH_WIDGET)
+
+       Now the error's `code` attribute can be used, both in server
+       and client code, to identify this type of error.  The data
+       (here, the widget name) is still embedded inside a translated
+       string, and can't be accessed by code.
+
+     * Fully machine-readable, with an error code and structured data:
+
+         class NoSuchWidgetError(JsonableError):
+             code = ErrorCode.NO_SUCH_WIDGET
+             data_fields = ['widget_name']
+
+             def __init__(self, widget_name):
+                 self.widget_name = widget_name  # type: str
+
+             @staticmethod
+             def msg_format():
+                 return _("No such widget: {widget_name}")
+
+         raise NoSuchWidgetError(widget_name)
+
+       Now both server and client code see a `widget_name` attribute.
+
+    Subclasses may also override `http_status_code`.
+    '''
+
+    # Override this in subclasses, or just pass a `code` argument
+    # to the JsonableError constructor.
+    code = ErrorCode.BAD_REQUEST  # type: ErrorCode
+
+    # Override this in subclasses if providing structured data.
+    data_fields = []  # type: List[str]
+
+    # Optionally override this in subclasses to return a different HTTP status,
+    # like 403 or 404.
     http_status_code = 400  # type: int
 
-    def __init__(self, msg):
-        # type: (Text) -> None
-        self.msg = msg
+    def __init__(self, msg, code=None):
+        # type: (Text, Optional[ErrorCode]) -> None
+        if code is not None:
+            self.code = code
+
+        # `_msg` is an implementation detail of `JsonableError` itself.
+        self._msg = msg  # type: Text
+
+    @staticmethod
+    def msg_format():
+        # type: () -> Text
+        '''Override in subclasses.  Gets the items in `data_fields` as format args.
+
+        This should return (a translation of) a string literal.
+        The reason it's not simply a class attribute is to allow
+        translation to work.
+        '''
+        # Secretly this gets one more format arg not in `data_fields`: `_msg`.
+        # That's for the sake of the `JsonableError` base logic itself, for
+        # the simplest form of use where we just get a plain message string
+        # at construction time.
+        return '{_msg}'
+
+    #
+    # Infrastructure -- not intended to be overridden in subclasses.
+    #
+
+    @property
+    def msg(self):
+        # type: () -> Text
+        format_data = dict(((f, getattr(self, f)) for f in self.data_fields),
+                           _msg=getattr(self, '_msg', None))
+        return self.msg_format().format(**format_data)
+
+    @property
+    def data(self):
+        # type: () -> Dict[str, Any]
+        return dict(((f, getattr(self, f)) for f in self.data_fields),
+                    code=self.code.name)
+
+    def to_json(self):
+        # type: () -> Dict[str, Any]
+        d = {'result': 'error', 'msg': self.msg}
+        d.update(self.data)
+        return d
 
     def __str__(self):
         # type: () -> str
-        return self.to_json_error_msg()  # type: ignore # remove once py3-only
-
-    def to_json_error_msg(self):
-        # type: () -> Text
-        return self.msg
+        return self.msg  # type: ignore # remove once py3-only
 
 class RateLimited(PermissionDenied):
     def __init__(self, msg=""):

zerver/lib/request.py

@@ -8,23 +8,30 @@ from six.moves import zip
 
 from django.utils.translation import ugettext as _
 
-from zerver.lib.exceptions import JsonableError
+from zerver.lib.exceptions import JsonableError, ErrorCode
 
 class RequestVariableMissingError(JsonableError):
-    def __init__(self, var_name):
-        self.var_name = var_name
+    code = ErrorCode.REQUEST_VARIABLE_MISSING
+    data_fields = ['var_name']
 
-    def to_json_error_msg(self):
-        return _("Missing '%s' argument") % (self.var_name,)
+    def __init__(self, var_name):
+        self.var_name = var_name  # type: str
+
+    @staticmethod
+    def msg_format():
+        return _("Missing '{var_name}' argument")
 
 class RequestVariableConversionError(JsonableError):
+    code = ErrorCode.REQUEST_VARIABLE_INVALID
+    data_fields = ['var_name', 'bad_value']
+
     def __init__(self, var_name, bad_value):
-        self.var_name = var_name
+        self.var_name = var_name  # type: str
         self.bad_value = bad_value
 
-    def to_json_error_msg(self):
-        return (_("Bad value for '%(var_name)s': %(value)s") %
-                {'var_name': self.var_name, 'value': self.bad_value})
+    @staticmethod
+    def msg_format():
+        return _("Bad value for '{var_name}': {bad_value}")
 
 # Used in conjunction with @has_request_variables, below
 class REQ(object):

zerver/middleware.py

@@ -286,7 +286,7 @@ class JsonErrorHandler(MiddlewareMixin):
     def process_exception(self, request, exception):
         # type: (HttpRequest, Exception) -> Optional[HttpResponse]
         if isinstance(exception, JsonableError):
-            return json_error(exception.to_json_error_msg(), status=exception.http_status_code)
+            return json_error(exception.msg, status=exception.http_status_code)
         if request.error_format == "JSON":
             logging.error(traceback.format_exc())
             return json_error(_("Internal server error"), status=500)

zerver/tests/test_i18n.py

@@ -86,7 +86,7 @@ class JsonTranslationTestCase(ZulipTestCase):
     @mock.patch('zerver.lib.request._')
     def test_json_error(self, mock_gettext):
         # type: (Any) -> None
-        dummy_value = "this arg is bad: '%s' (translated to German)"
+        dummy_value = "this arg is bad: '{var_name}' (translated to German)"
         mock_gettext.return_value = dummy_value
 
         email = self.example_email('hamlet')

zerver/tornado/event_queue.py

@@ -539,7 +539,7 @@ def fetch_events(query):
             logging.info("Disconnected handler for queue %s (%s/%s)" % (queue_id, user_profile_email,
                                                                         client_type_name))
     except JsonableError as e:
-        return dict(type="error", handler_id=handler_id, message=e.to_json_error_msg())
+        return dict(type="error", handler_id=handler_id, message=e.msg)
 
     client.connect_handler(handler_id, client_type_name)
     return dict(type="async")

zerver/views/messages.py

@@ -10,9 +10,10 @@ from django.http import HttpRequest, HttpResponse
 from typing import Dict, List, Set, Text, Any, AnyStr, Callable, Iterable, \
     Optional, Tuple, Union
 from zerver.lib.str_utils import force_text
+from zerver.lib.exceptions import JsonableError, ErrorCode
 from zerver.lib.html_diff import highlight_html_differences
 from zerver.decorator import authenticated_json_post_view, has_request_variables, \
-    REQ, JsonableError, to_non_negative_int
+    REQ, to_non_negative_int
 from django.utils.html import escape as escape_html
 from zerver.lib import bugdown
 from zerver.lib.actions import recipient_for_emails, do_update_message_flags, \
@@ -56,13 +57,17 @@ import six
 LARGER_THAN_MAX_MESSAGE_ID = 10000000000000000
 
 class BadNarrowOperator(JsonableError):
+    code = ErrorCode.BAD_NARROW
+    data_fields = ['desc']
+
     def __init__(self, desc):
         # type: (str) -> None
-        self.desc = desc
+        self.desc = desc  # type: str
 
-    def to_json_error_msg(self):
+    @staticmethod
+    def msg_format():
         # type: () -> str
-        return _('Invalid narrow operator: {}').format(self.desc)
+        return _('Invalid narrow operator: {desc}')
 
 Query = Any  # TODO: Should be Select, but sqlalchemy stubs are busted
 ConditionTransform = Any  # TODO: should be Callable[[ColumnElement], ColumnElement], but sqlalchemy stubs are busted

zerver/views/streams.py

@@ -6,7 +6,8 @@ from django.conf import settings
 from django.db import transaction
 from django.http import HttpRequest, HttpResponse
 
-from zerver.lib.request import JsonableError, REQ, has_request_variables
+from zerver.lib.exceptions import JsonableError, ErrorCode
+from zerver.lib.request import REQ, has_request_variables
 from zerver.decorator import authenticated_json_post_view, \
     authenticated_json_view, require_realm_admin, to_non_negative_int
 from zerver.lib.actions import bulk_remove_subscriptions, \
@@ -35,16 +36,18 @@ from six.moves import urllib
 import six
 
 class PrincipalError(JsonableError):
+    code = ErrorCode.UNAUTHORIZED_PRINCIPAL
+    data_fields = ['principal']
     http_status_code = 403
 
     def __init__(self, principal):
         # type: (Text) -> None
         self.principal = principal  # type: Text
 
-    def to_json_error_msg(self):
+    @staticmethod
+    def msg_format():
         # type: () -> Text
-        return ("User not authorized to execute queries on behalf of '%s'"
-                % (self.principal,))
+        return _("User not authorized to execute queries on behalf of '{principal}'")
 
 def principal_to_user_profile(agent, principal):
     # type: (UserProfile, Text) -> UserProfile