Commit bd6471f0e3 (#28691) added this
reference to the old name, even though it had already been renamed in
commit b220d29fed (#17775), presumably
because that had failed to update the OpenAPI description.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Having a non-identity `cache_transformer` is no different from running
it on every row of the query_function. Simplify understanding of the
codepath used in caching by merging the pieces of code.
Rather than pass around a list of message objects in-memory, we
instead keep the same constructed QuerySet which includes the later
propagated messages (if any), and use that same query to pick out
affected Attachment objects, rather than limiting to the set of ids.
This is not necessarily a win -- the list of message-ids *may* be very
long, and thus the query may be more concise, easier to send to
PostgreSQL, and faster for PostgreSQL to parse. However, the list of
ids is almost certainly better-indexed.
After processing the move, the QuerySet must be re-defined as a search
of ids (and possibly a very long list of such), since there is no
other way which is guaranteed to correctly single out the moved
messages. At this point, it is mostly equivalent to the list of
Message objects, and certainly takes no less memory.
Rather than use `bulk_update()` to batch-move chunks of messages, use
a single SQL query to move the messages. This is much more efficient
for large topic moves. Since the `edit_history` field is not yet
JSON (see #26496) this requires that PostgreSQL cast the current data
into `jsonb`, append the new data (also cast to `jsonb`), and then
re-cast that as text.
For single-message moves, this _increases_ the SQL query count by one,
since we have to re-query for the updated data from the database after
the bulk update. However, this is overall still a performance
improvement, which improves to 2x or 3x for larger topic moves. Below
is a table of duration in seconds to run `do_update_message` to move a
topic to a new stream, based on messages in the topic, for before and
after this change:
| Topic size | Before | After |
| ---------- | -------- | ------- |
| 1 | 0.1036 | 0.0868 |
| 2 | 0.1108 | 0.0925 |
| 5 | 0.1139 | 0.0959 |
| 10 | 0.1218 | 0.0972 |
| 20 | 0.1310 | 0.1098 |
| 50 | 0.1759 | 0.1366 |
| 100 | 0.2307 | 0.1662 |
| 200 | 0.3880 | 0.2229 |
| 500 | 0.7676 | 0.4052 |
| 1000 | 1.3990 | 0.6848 |
| 2000 | 2.9706 | 1.3370 |
| 5000 | 7.5218 | 3.2882 |
| 10000 | 14.0272 | 5.4434 |
This applies access restrictions in SQL, so that individual messages
do not need to be walked one-by-one. It only functions for stream
messages.
Use of this method significantly speeds up checks if we moved "all
visible messages" in a topic, since we no longer need to walk every
remaining message in the old topic to determine that at least one was
visible to the user. Similarly, it significantly speeds up merging
into existing topics, since it no longer must walk every message in
the new topic to determine if the user could see at least one.
Finally, it unlocks the ability to bulk-update only messages the user
has access to, in a single query (see subsequent commit).
This is a preparatory commit that refactors the check_update_message
method to extract the checks containing whether a user can edit the
message or not into a separate method -validate_message_content_edit,
so that it can be re used later.
This logic was apparently missed when we implemented private streams
with shared history; the correct check is to look at whether the user
can access message history in the stream, which used to be equivalent
to whether it's a private stream.
The problem was that earlier this was just an uncaught JsonableError,
leading to a full traceback getting spammed to the admins.
The prior commit introduced a clear .code for this error on the bouncer
side, meaning the self-hosted server can now detect that and handle it
nicely, by just logging.error about it and also take the opportunity to
adjust the realm.push_notifications_... flags.
This commit removes the stale 'email_gateway' parameter
from 'do_send_messages' function.
This should have been removed in 6c473ed75f,
when the call to 'build_message_send_dict' was removed
from 'do_send_messages'.
This error message didn’t make sense for the check as written, and our
OpenAPI document already provides the expected format for our 200
responses.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Real requests would not validate against the previous version. There
seems to be no consistent way to determine whether a string parameter
should be coerced to an integer for validation against an allOf
schema (which works at the level of JSON objects, not strings).
See also https://github.com/python-openapi/openapi-core/issues/698.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The endpoint was lacking validation that the authentication_methods dict
submitted by the user made sense. So e.g. it allowed submitting a
nonsense key like NoSuchBackend or modifying the realm's configured
authentication methods for a backend that's not enabled on the server,
which should not be allowed.
Both were ultimately harmless, because:
1. Submitting NoSuchBackend would luckily just trigger a KeyError inside
the transaction.atomic() block in do_set_realm_authentication_methods
so it would actually roll back the database changes it was trying to
make. So this couldn't actually create some weird
RealmAuthenticationMethod entries.
2. Silently enabling or disabling e.g. GitHub for a realm when GitHub
isn't enabled on the server doesn't really change anything. And this
action is only available to the realm's admins to begin with, so
there's no attack vector here.
test_supported_backends_only_updated wasn't actually testing anything,
because the state it was asserting:
```
self.assertFalse(github_auth_enabled(realm))
self.assertTrue(dev_auth_enabled(realm))
self.assertFalse(password_auth_enabled(realm))
```
matched the desired state submitted to the API...
```
result = self.client_patch(
"/json/realm",
{
"authentication_methods": orjson.dumps(
{"Email": False, "Dev": True, "GitHub": False}
).decode()
},
)
```
so we just replace it with a new test that tests the param validation.
- Renames "Bots and integrations" to "Bots overview" everywhere
(sidebar, page title, page URL).
- Adds a copy of /api/integrations-overview (symbolic link) as the
second page in the Bots & integrations section, titled
"Integrations overview".
Fixes#28758.
We use Alertmanager as an aggregation place for example for failing CI pipelines,
and `graph` does not always reflect the source of the alert. It's called `source` originally
and I think it should stay this way.
Alex Vandiver
Alya Abbott
Anders Kaseorg
Tim Abbott
Karl Stolley
Lauryn Menard
roanster007
Mateusz Mandera
David Rosa
shashank-23002
Prakhar Pratyush
Artur Szcześniak