Since this class was built, folks have always chosen
to subclass JsonableError for situations where
the default of ErrorCode.BAD_REQUEST is insufficient.
So now we simplify the use cases, which also gets
us 100% coverage on this core module.
UnexpectedWebhookEventType is a generic exception that we may
now raise when we encounter a webhook event that is new or one
that we simply aren't aware of.
webhook-errors.log file is cluttered with Stream.DoesNotExist
errors, which hides the errors that we actually need to see. So,
since check_message already sends the bot_owner a PM if the webhook
bot tries to send a message to a non-existent stream, we can ignore
such exceptions.
This allows us to reliably parse the error in code, rather than
attempt to parse the error text. Because the error text gets
translated into the user's language, this error-handling path
wasn't functioning at all for users using Zulip in any of the
seven non-English languages for which we had a translation for
this string.
Together with 709c3b50f which fixed a similar issue in a
different error-handling path, this fixes#5598.
This fixes the original issue that #5598 was the root cause of; when
the user returns to a Zulip browser tab after they've been idle past
the timeout (10 min, per IDLE_EVENT_QUEUE_TIMEOUT_SECS), we now
correctly reload the page even if they're using Zulip in German or
another non-English language where we have a translation for the
relevant error message.
All JsonableError subclasses now have corresponding ErrorCode values
of their own, reducing the number of different patterns for using
the new JsonableError API.
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.
This simplifies things for all codepaths not involving this feature.
Using this feature becomes slightly easier when you're already
defining a subclass, but now requires you to define a subclass.
Currently we use it just once out of >100 uses of JsonableError, and
that use already has a subclass, so this seems like a win.
With #5598 there will soon be an application-level error code
optionally associated with a `JsonableError`, so rename this
field to make clear that it specifically refers to an
HTTP status code.
Also take this opportunity to eliminate most of the places
that refer to it, which only do so to repeat the default value.
The file `zerver/lib/request.py` doesn't have type annotations
of its own; if they did, they would duplicate the annotations that
exist in its stub file `zerver/lib/request.pyi`. The latter exists
so that we can provide types for the highly dynamic `REQ` and
`has_request_variables`, which are beyond the type-checker's ken
to type-check, but we should minimize the scope of code that gets
that kind of treatment and `JsonableError` is not at all the sort of
code that needs it.
So move the definition of `JsonableError` into a file that does
get type-checked.
In doing so, the type-checker points out one issue already:
`__str__` should return a `str`, but we had it returning a `Text`,
which on Python 2 is not the same thing. Indeed, because the
message we pass to the `JsonableError` constructor is generally
translated, it may well be a Unicode string stuffed full of
non-ASCII characters. This is potentially a bit of a landmine.
But (a) it can only possibly matter in Python 2 which we intend to
be off before long, and (b) AFAIK it hasn't been biting us in
practice, so we've probably reasonably well worked around it where
it could matter. Leave it as is.
Steve Howell
Shubham Dhama
Eeshan Garg
jkiely
Tim Abbott
Aditya Bansal
Vishnu Ks
rht
Greg Price