As premonitioned in c741c527d7, it is
indeed possible for `get_handler_by_id` to error out by cause the
handler has been unset elsewhere.
Protect the callsites of `get_handler_by_id` to be able to gracefully
handle when the handler has already done away.
4af00f61a8 claimed that `on_finish` and
`on_connection_close` were mutually exclusive. In cases where a
`DELETE` is called on the queue while a longpoll is in progress, this
can cause _both_ to happen:
- The `DELETE` pushes a `cleanup_queue` event, which triggers
`finish_handler` to begin pushing out an empty event response to the
longpoll connection.
- In the midst of that, in an `await`, the longpoll connection drops,
and `on_connection_close` clears the handler.
- The `await` resumes, calls `finish`, and attempts to clear the
handler.
The easiest solution is to make `clear_handler_by_id` tolerant to
multiple attempts to clear it. Since these processes run in parallel,
it means that parts may have a `handler_id` but `get_handler_by_id`
may error in attempting to look it up. We have not observed this in
testing, and I cannot currently prove it is impossible.
This partially reverts 579bdc18f85ea8599c8cf1f53ddb02fd41d97993; it
assumed (based on its documentation) that `on_finish` was called for
all requests, even client-terminated ones. This is not accurate; it
is only called when the request calls `finish`, which only happens for
successful requests. This caused every client-closed connection to
leak a handler (ironically, exactly re-introducing the bug previously
fixed in 12a5a3a6e1).
This behaviour was obscured by the development environment's proxy;
see comment added in the previous commit.
Instead of replacing the `clear_handler_by_id` call into
`ClientDescriptor.disconnect_handler`, we instead place it on
`AsyncDjangoHandler.on_connection_close`. This is more correct for
a few reasons:
- `on_connection_close` will be called if the client goes away during
a request without a client descriptor. If the handler garbage
collection of handlers runs inside the ClientDescriptor, we leak
handlers.
- `disconnect_handler` also runs when successfully sending an event,
which already calls `on_finish`. We avoid double-calling
`clear_handler_by_id` by doing it in two clearly exclusive cases,
`on_finish` and `on_connection_close`.
- It combines the creation and garbage collection logic into one
file, decreasing action at a distance which causes memory leaks.
This commit adds code to not include original details of senders like
name, email and avatar url in the message objects sent through events
and in the response of endpoint used to fetch messages.
This is the last major commit for the project to add support for
limiting guest access to an entire organization.
Fixes#10970.
initialize() is called on every request, and stored the
`RequestHandler` (and thus `HTTPServerRequest`) in a global shared
dict. However, the object is only removed from that structure if the
request was successful. This means that failed requests (such as 405
Method Not Allowed) leaked `RequestHandler`s and
`HTTPServerRequest`s.
Move the cleanup to `on_finish`, which is called at the close of all
requests, async and not, successful or not.
These are not part of the API, and lead to moderately confusing
behaviour -- they block (or not) requested, but of course send no
actual data in the body.
This commit renames the keyword 'pm' to 'dm' in the
'pm_mention_email_disabled_user_ids' and
'pm_mention_push_disabled_user_ids' attributes of the
'RecipientInfoResult' dataclass.
'pm' and 'dm' are the acronyms for 'private message' and
'direct message' respectively.
It includes 'TODO/compatibility' code to support the old format
fields in the tornado queues during the Zulip server upgrades.
This commit completes the notifications part of the @topic
wildcard mention feature.
Notifications are sent to the topic participants for the
@topic wildcard mention.
This prep commit replaces the 'wildcard' keyword in the codebase
with 'stream_wildcard' at some places for better readability, as
we plan to introduce 'topic_wildcards' as a part of the
'@topic mention' project.
Currently, 'wildcards = ["all", "everyone", "stream"]' which is an
alias to mention everyone in the stream, hence better renamed as
'stream_wildcards'.
Eventually, we will have:
'stream_wildcard' as an alias to mention everyone in the stream.
'topic_wildcard' as an alias to mention everyone in the topic.
'wildcard' refers to 'stream_wildcard' and 'topic_wildcard' as a whole.
This is a first step toward two goals:
* support dictionary-like narrows when registering events
* use readable dataclasses internally
This is gonna be a somewhat complicated exercise due to how
events get serialized, but fortunately this interim step
doesn't require any serious shims, so it improves the codebase
even if the long-term goals may take a while to get sorted
out.
The two places where we have to use a helper to convert narrows
from tuples to dataclasses will eventually rely on their callers
to do the conversion, but I don't want to re-work the entire
codepath yet.
Note that the new NarrowTerm dataclass makes it more explicit
that the internal functions currently either don't care about
negated flags or downright don't support them. This way mypy
protects us from assuming that we can just add negated support
at the outer edges.
OTOH I do make a tiny effort here to slightly restructure
narrow_filter in a way that paves the way for negation support.
The bigger goal by far, though, is to at least support the
dictionary format.
We no longer pass in a big opaque event to narrow_filter
(which is inside build_narrow_filter). We instead explicitly
pass in message and flags. This leads to a bit more type
safety, and it's also more flexible. There's no reason to
build an entire event just to see if a message belongs to
a narrow.
The changes to the test work around the fact that the fixtures
are sloppy with types. I plan a subsequent commit to clean
up those tests significantly.
django-stubs 4.2.1 gives transaction.on_commit a more accurate type
annotation, but this exposed that mypy can’t handle the lambda default
parameters that we use to recapture loop variables such as
for stream_id in public_stream_ids:
peer_user_ids = …
event = …
transaction.on_commit(
lambda event=event, peer_user_ids=peer_user_ids: send_event(
realm, event, peer_user_ids
)
)
https://github.com/python/mypy/issues/15459
A workaround that mypy accepts is
transaction.on_commit(
(
lambda event, peer_user_ids: lambda: send_event(
realm, event, peer_user_ids
)
)(event, peer_user_ids)
)
But that’s kind of ugly and potentially error-prone, so let’s make a
helper function for this very common pattern.
send_event_on_commit(realm, event, peer_user_ids)
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit makes it possible for users to control the wildcard
mention notifications for messages sent to followed topics
via a global notification setting.
There is no support for configuring this setting
through the UI yet.
This commit makes it possible for users to control
the push notifications for messages sent to followed topics
via a global notification setting.
There is no support for configuring this setting
through the UI yet.
This commit makes it possible for users to control
the email notifications for messages sent to followed topics
via a global notification setting.
Although there is no support for configuring this setting
through the UI yet.
Add five new fields to the UserBaseSettings class for
the "followed topic notifications" feature, similar to
stream notifications. But this commit consists only of
the implementation of email notifications.
In #23380 we want to replace all occurrences of `uri` with `url`.
This commit replaces the occurrences appeared in a variable name
`tornado_uri` and a function name `get_tornado_uri`.
This swaps out url_format_string from all of our APIs and replaces it
with url_template. Note that the documentation changes in the following
commits will be squashed with this commit.
We change the "url_format" key to "url_template" for the
realm_linkifiers events in event_schema, along with updating
LinkifierDict. "url_template" is the name chosen to normalize
mixed usages of "url_format_string" and "url_format" throughout
the backend.
The markdown processor is updated to stop handling the format string
interpolation and delegate the task template expansion to the uri_template
library instead.
This change affects many test cases. We mostly just replace "%(name)s"
with "{name}", "url_format_string" with "url_template" to make sure that
they still pass. There are some test cases dedicated for testing "%"
escaping, which aren't relevant anymore and are subject to removal.
But for now we keep most of them as-is, and make sure that "%" is always
escaped since we do not use it for variable substitution any more.
Since url_format_string is not populated anymore, a migration is created
to remove this field entirely, and make url_template non-nullable since
we will always populate it. Note that it is possible to have
url_template being null after migration 0422 and before 0424, but
in practice, url_template will not be None after backfilling and the
backend now is always setting url_template.
With the removal of url_format_string, RealmFilter model will now be cleaned
with URL template checks, and the old checks for escapes are removed.
We also modified RealmFilter.clean to skip the validation when the
url_template is invalid. This avoids raising mulitple ValidationError's
when calling full_clean on a linkifier. But we might eventually want to
have a more centric approach to data validation instead of having
the same validation in both the clean method and the validator.
Fixes#23124.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Refactors instances of `message_type_name` and `message_type`
that are referring to API message type value ("stream" or
"private") to use `recipient_type_name` instead.
Prep commit for adding "direct" as a value for endpoints with a
`type` parameter to indicate whether the message is a stream or
direct message.
This code is called in the hot path when Tornado is processing events.
As such, making this code performant is important. Profiling shows
that a significant portion of the time is spent calling asdict() to
serialize the UserMessageNotificationsData dataclass. In this case
`asdict` does several steps which we do not need, such as attempting
to recurse into its fields, and deepcopy'ing the values of the fields.
In our use case, these add a notable amount of overhead:
```py3
from zerver.tornado.event_queue import UserMessageNotificationsData
from dataclasses import asdict
from timeit import timeit
o = UserMessageNotificationsData(1, False, False, False, False, False, False, False, False, False, False, False)
%timeit asdict(o)
%timeit {**vars(o)}
```
Replace the `asdict` call with a direct access of the fields. We
perform a shallow copy because we do need to modify the resulting
fields.
Black 23 enforces some slightly more specific rules about empty line
counts and redundant parenthesis removal, but the result is still
compatible with Black 22.
(This does not actually upgrade our Python environment to Black 23
yet.)
Signed-off-by: Anders Kaseorg <anders@zulip.com>
A missed message email notification, where the message is the welcome
message sent by the welcome bot on account creation, get sent when
the user somehow not focuses the browser tab during account creation.
No missed message email or push notifications should be sent for the
messages generated by the welcome bot.
'internal_send_private_message' accepts a parameter
'disable_external_notifications' and is set to 'True' when the sender
is 'welcome bot'.
A check is introduced in `trivially_should_not_notify`, not to notify
if `disable_external_notifications` is true.
TestCases are updated to include the `disable_external_notifications`
check in the early (False) return patterns of `is_push_notifiable` and
`is_email_notifiable`.
One query reduced for both `test_create_user_with_multiple_streams`
and `test_register`.
Reason: When welcome bot sends message after user creation
`do_send_messages` calls `get_active_presence_idle_user_ids`,
`user_ids` in `get_active_presence_idle_user_ids` remains empty if
`disable_external_notifications` is true because `is_notifiable` returns
false.
`get_active_presence_idle_user_ids` calls `filter_presence_idle_user_ids`
and since the `user_ids` is empty, the query inside the function doesn't
get executed.
MissedMessageHookTest updated.
Fixes: #22884
Doing rapid pace mark-as-unread in the Zulip web application, one
observed assertion failures showing that the server would send an
event containing multiple message IDs but only one of the messages
present in the message_details side data structure.
The cause of this was the "virtual events" compression system; two
flags/remove/read events were being combined by simply concatenating
the lists of events, without any attempt to merge the
`message_details` field on those objects.
The immediate fix is to disable virtual events compression for this
event class, but it's not unlikely we'll need to just eliminate the
virtual_events system entirely, because it seems difficult to make it
soundly handle a message whose state for a given flag changes back and
forth while the client is offline.
But we'll leave that for later, since removing that optimization
deserves more discussion than fixing this event corruption bug.
return inside finally blocks causes exceptions to be silenced.
Although these blocks follow blanket ‘except Exception’ handlers, they
do not seem to have a goal of silencing BaseException and exceptions
thrown by the exception handler, so rewrite them to avoid it.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Mobile clients older than v27.192 do not support PRONOUNS type
custom profile fields, so we instead change the type of it to
SHORT_TEXT in the data sent with register response and also in
the events sent to those clients.
Anders Kaseorg
Alex Vandiver
Sahil Batra
Prakhar Pratyush
Steve Howell
Lauryn Menard
AcKindle3
Zixuan James Li
Tim Abbott