From 37660dd0e78d4888aa575f69176de848b8bd510f Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 9 Aug 2023 22:09:25 -0400 Subject: [PATCH] linkifier: Support reordering linkifiers. This adds API support to reorder linkifiers and makes sure that the returned lists of linkifiers from `GET /events`, `POST /register`, and `GET /realm/linkifiers` are always sorted with the order that they should processed when rendering linkifiers. We set the new `order` field to the ID with the migration. This preserves the order of the existing linkifiers. New linkifiers added will always be ordered the last. When reordering, the `order` field of all linkifiers in the same realm is updated, in a manner similar to how we implement ordering for `custom_profile_fields`. --- api_docs/changelog.md | 5 + api_docs/include/rest-endpoints.md | 1 + version.py | 2 +- zerver/actions/realm_linkifiers.py | 65 ++++++++++- zerver/migrations/0466_realmfilter_order.py | 28 +++++ zerver/models.py | 6 +- zerver/openapi/python_examples.py | 13 +++ zerver/openapi/zulip.yaml | 58 +++++++++- zerver/tests/test_events.py | 12 ++ zerver/tests/test_markdown.py | 22 ++-- zerver/tests/test_realm_linkifiers.py | 120 +++++++++++++++++++- zerver/views/realm_linkifiers.py | 15 +++ zproject/urls.py | 3 +- 13 files changed, 333 insertions(+), 17 deletions(-) create mode 100644 zerver/migrations/0466_realmfilter_order.py diff --git a/api_docs/changelog.md b/api_docs/changelog.md index c1c9408d33..5fc916383a 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,11 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 202** + +* [`PATCH /realm/linkifiers`](/api/reorder-linkifiers): Added new endpoint + to support changing the order in which linkifiers will be processed. + **Feature level 201** * [`POST /zulip-outgoing-webhook`]: Renamed the notification trigger diff --git a/api_docs/include/rest-endpoints.md b/api_docs/include/rest-endpoints.md index 9e2b508eda..449e5658cd 100644 --- a/api_docs/include/rest-endpoints.md +++ b/api_docs/include/rest-endpoints.md @@ -92,6 +92,7 @@ * [Add a linkifier](/api/add-linkifier) * [Update a linkifier](/api/update-linkifier) * [Remove a linkifier](/api/remove-linkifier) +* [Reorder linkifiers](/api/reorder-linkifiers) * [Add a code playground](/api/add-code-playground) * [Remove a code playground](/api/remove-code-playground) * [Get all custom emoji](/api/get-custom-emoji) diff --git a/version.py b/version.py index eb9b56e3ef..70dd1e1492 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 201 +API_FEATURE_LEVEL = 202 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/actions/realm_linkifiers.py b/zerver/actions/realm_linkifiers.py index 5154b78a33..7daf00ea49 100644 --- a/zerver/actions/realm_linkifiers.py +++ b/zerver/actions/realm_linkifiers.py @@ -2,8 +2,11 @@ from typing import Dict, List, Optional import orjson from django.db import transaction +from django.db.models import Max from django.utils.timezone import now as timezone_now +from django.utils.translation import gettext as _ +from zerver.lib.exceptions import JsonableError from zerver.lib.types import LinkifierDict from zerver.models import ( Realm, @@ -11,6 +14,7 @@ from zerver.models import ( RealmFilter, UserProfile, active_user_ids, + flush_linkifiers, linkifiers_for_realm, ) from zerver.tornado.django_api import send_event_on_commit @@ -35,7 +39,15 @@ def do_add_linkifier( ) -> int: pattern = pattern.strip() url_template = url_template.strip() - linkifier = RealmFilter(realm=realm, pattern=pattern, url_template=url_template) + # This makes sure that the new linkifier is always ordered the last modulo + # the rare race condition. + max_order = RealmFilter.objects.aggregate(Max("order"))["order__max"] + if max_order is None: + linkifier = RealmFilter(realm=realm, pattern=pattern, url_template=url_template) + else: + linkifier = RealmFilter( + realm=realm, pattern=pattern, url_template=url_template, order=max_order + 1 + ) linkifier.full_clean() linkifier.save() @@ -131,3 +143,54 @@ def do_update_linkifier( ) notify_linkifiers(realm, realm_linkifiers) + + +@transaction.atomic(durable=True) +def check_reorder_linkifiers( + realm: Realm, ordered_linkifier_ids: List[int], *, acting_user: Optional[UserProfile] +) -> None: + """ordered_linkifier_ids should contain ids of all existing linkifiers. + In the rare situation when any of the linkifier gets deleted that more ids + are passed, the checks below are sufficient to detect inconsistencies most of + the time.""" + # Repeated IDs in the user request would collapse into the same key when + # constructing the set. + linkifier_id_set = set(ordered_linkifier_ids) + if len(linkifier_id_set) < len(ordered_linkifier_ids): + raise JsonableError(_("The ordered list must not contain duplicated linkifiers")) + + linkifiers = RealmFilter.objects.filter(realm=realm) + if {linkifier.id for linkifier in linkifiers} != linkifier_id_set: + raise JsonableError( + _("The ordered list must enumerate all existing linkifiers exactly once") + ) + + # After the validation, we are sure that there is nothing to do. Return + # early to avoid flushing the cache and populating the audit logs. + if len(linkifiers) == 0: + return + + id_to_new_order = { + linkifier_id: order for order, linkifier_id in enumerate(ordered_linkifier_ids) + } + + for linkifier in linkifiers: + assert linkifier.id in id_to_new_order + linkifier.order = id_to_new_order[linkifier.id] + RealmFilter.objects.bulk_update(linkifiers, fields=["order"]) + flush_linkifiers(instance=linkifiers[0]) + + # This roundtrip re-fetches the linkifiers sorted in the new order. + realm_linkifiers = linkifiers_for_realm(realm.id) + RealmAuditLog.objects.create( + realm=realm, + acting_user=acting_user, + event_type=RealmAuditLog.REALM_LINKIFIERS_REORDERED, + event_time=timezone_now(), + extra_data=orjson.dumps( + { + "realm_linkifiers": realm_linkifiers, + } + ).decode(), + ) + notify_linkifiers(realm, realm_linkifiers) diff --git a/zerver/migrations/0466_realmfilter_order.py b/zerver/migrations/0466_realmfilter_order.py new file mode 100644 index 0000000000..f60f6d5f67 --- /dev/null +++ b/zerver/migrations/0466_realmfilter_order.py @@ -0,0 +1,28 @@ +# Generated by Django 4.2.3 on 2023-08-09 18:48 + +from django.db import migrations, models +from django.db.backends.base.schema import BaseDatabaseSchemaEditor +from django.db.migrations.state import StateApps +from django.db.models import F + + +def migrate_set_order_value(apps: StateApps, schema_editor: BaseDatabaseSchemaEditor) -> None: + RealmFilter = apps.get_model("zerver", "RealmFilter") + RealmFilter.objects.all().update(order=F("id")) + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0465_backfill_scheduledmessagenotificationemail_trigger"), + ] + + operations = [ + migrations.AddField( + model_name="realmfilter", + name="order", + field=models.IntegerField(default=0), + ), + migrations.RunPython( + migrate_set_order_value, reverse_code=migrations.RunPython.noop, elidable=True + ), + ] diff --git a/zerver/models.py b/zerver/models.py index f5d4314112..81947764f9 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1246,6 +1246,9 @@ class RealmFilter(models.Model): realm = models.ForeignKey(Realm, on_delete=CASCADE) pattern = models.TextField() url_template = models.TextField(validators=[url_template_validator]) + # Linkifiers are applied in a message/topic in order; the processing order + # is important when there are overlapping patterns. + order = models.IntegerField(default=0) class Meta: unique_together = ("realm", "pattern") @@ -1309,7 +1312,7 @@ def linkifiers_for_realm(realm_id: int) -> List[LinkifierDict]: url_template=linkifier.url_template, id=linkifier.id, ) - for linkifier in RealmFilter.objects.filter(realm_id=realm_id).order_by("id") + for linkifier in RealmFilter.objects.filter(realm_id=realm_id).order_by("order") ] @@ -4499,6 +4502,7 @@ class AbstractRealmAuditLog(models.Model): REALM_LINKIFIER_REMOVED = 225 REALM_EMOJI_ADDED = 226 REALM_EMOJI_REMOVED = 227 + REALM_LINKIFIERS_REORDERED = 228 SUBSCRIPTION_CREATED = 301 SUBSCRIPTION_ACTIVATED = 302 diff --git a/zerver/openapi/python_examples.py b/zerver/openapi/python_examples.py index 682fe35d3d..161c065237 100644 --- a/zerver/openapi/python_examples.py +++ b/zerver/openapi/python_examples.py @@ -392,6 +392,18 @@ def get_realm_linkifiers(client: Client) -> None: validate_against_openapi_schema(result, "/realm/linkifiers", "get", "200") +@openapi_test_function("/realm/linkifiers:patch") +def reorder_realm_linkifiers(client: Client) -> None: + # {code_example|start} + # Reorder the linkifiers in the user's organization. + order = [4, 3, 2, 1] + request = {"ordered_linkifier_ids": json.dumps(order)} + + result = client.call_endpoint(url="/realm/linkifiers", method="PATCH", request=request) + # {code_example|end} + validate_against_openapi_schema(result, "/realm/linkifiers", "patch", "200") + + @openapi_test_function("/realm/profile_fields:get") def get_realm_profile_fields(client: Client) -> None: # {code_example|start} @@ -1629,6 +1641,7 @@ def test_server_organizations(client: Client) -> None: update_realm_filter(client) add_realm_playground(client) get_server_settings(client) + reorder_realm_linkifiers(client) remove_realm_filter(client) remove_realm_playground(client) get_realm_emoji(client) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 3ec6515a6c..ab2286b27d 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3205,8 +3205,16 @@ paths: realm_linkifiers: type: array description: | - Array of dictionaries where each dictionary contains details about - a single realm linkifier. + An ordered array of dictionaries where each + dictionary contains details about a single + realm linkifier. + + The order of the array reflects the order + that each linkifier should be processed when + linkifying messages and topics. By default, + new linkifiers are ordered last. This order + can be modified with [`PATCH + /realm/linkifiers`](/api/reorder-linkifiers). items: type: object additionalProperties: false @@ -10606,7 +10614,14 @@ paths: linkifiers: type: array description: | - An array of objects, where each object describes a linkifier. + An ordered array of objects, where each object + describes a linkifier. + + The order of the array reflects the order that each + linkifier should be processed when linkifying messages + and topics. By default, new linkifiers are ordered + last. This order can be modified with [`PATCH + /realm/linkifiers`](/api/reorder-linkifiers). items: type: object additionalProperties: false @@ -10641,6 +10656,35 @@ paths: ], "result": "success", } + patch: + operationId: reorder-linkifiers + summary: Reorder all linkifiers + tags: ["server_and_organizations"] + description: | + Change the order that the regular expression patterns in the organization's + [linkifiers](/help/add-a-custom-linkifier) are matched in messages and topics. + Useful when defining linkifiers with overlapping patterns. + + **Changes**: New in Zulip 8.0 (feature level 202). Before this feature level, + linkifiers were always processed in order by ID, which meant users would + need to delete and recreate them to reorder the list of linkifiers. + parameters: + - name: ordered_linkifier_ids + in: query + description: | + A list of the IDs of all the linkifiers defined in this + organization, in the desired new order. + content: + application/json: + schema: + type: array + items: + type: integer + example: [3, 2, 1, 5] + required: true + responses: + "200": + $ref: "#/components/responses/SimpleSuccess" /realm/filters: post: operationId: add-linkifier @@ -11382,9 +11426,15 @@ paths: description: | Present if `realm_linkifiers` is present in `fetch_event_types`. - Array of objects where each object describes a single + An ordered array of objects where each object describes a single [linkifier](/help/add-a-custom-linkifier). + The order of the array reflects the order that each + linkifier should be processed when linkifying messages + and topics. By default, new linkifiers are ordered + last. This order can be modified with [`PATCH + /realm/linkifiers`](/api/reorder-linkifiers). + Clients will receive an empty array unless the event queue is registered with the client capability `{"linkifier_url_template": true}`. See [`client_capabilities`](/api/register-queue#parameter-client_capabilities) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 62bab3c048..0b9d9e2a11 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -62,6 +62,7 @@ from zerver.actions.realm_domains import ( from zerver.actions.realm_emoji import check_add_realm_emoji, do_remove_realm_emoji from zerver.actions.realm_icon import do_change_icon_source from zerver.actions.realm_linkifiers import ( + check_reorder_linkifiers, do_add_linkifier, do_remove_linkifier, do_update_linkifier, @@ -2179,6 +2180,17 @@ class NormalActionsTest(BaseAction): ) check_realm_linkifiers("events[0]", events[0]) + linkifier_ids = list( + RealmFilter.objects.all().values_list("id", flat=True).order_by("order") + ) + events = self.verify_action( + lambda: check_reorder_linkifiers( + self.user_profile.realm, [linkifier_ids[-1], *linkifier_ids[:-1]], acting_user=None + ), + num_events=1, + ) + check_realm_linkifiers("events[0]", events[0]) + events = self.verify_action( lambda: do_remove_linkifier(self.user_profile.realm, regex, acting_user=None), num_events=1, diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index bf4525b272..cf3f43246e 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -1398,21 +1398,25 @@ class MarkdownTest(ZulipTestCase): realm=realm, pattern="http", url_template="http://example.com/", + order=1, ), RealmFilter( realm=realm, pattern="b#(?P[a-z]+)", url_template="http://example.com/b/{id}", + order=2, ), RealmFilter( realm=realm, pattern="a#(?P[a-z]+) b#(?P[a-z]+)", url_template="http://example.com/a/{aid}/b/{bid}", + order=3, ), RealmFilter( realm=realm, pattern="a#(?P[a-z]+)", url_template="http://example.com/a/{id}", + order=4, ), ], [ @@ -1454,20 +1458,22 @@ class MarkdownTest(ZulipTestCase): def test_linkifier_precedence(self) -> None: realm = self.example_user("hamlet").realm RealmFilter.objects.filter(realm=realm).delete() - # The insertion order should not affect the fact that the linkifiers are ordered by id. - # Note that we might later switch to a different field to order the linkifiers. - sequence = (10, 3, 11, 2, 4, 5, 6) - for cur_precedence in sequence: + # The insertion order should not affect the fact that the linkifiers are + # ordered by the `order` field. + order_values = (10, 3, 11, 2, 4, 5, 6) + order_to_id = {} + for cur_order in order_values: linkifier = RealmFilter( - id=cur_precedence, realm=realm, - pattern=f"abc{cur_precedence}", + pattern=f"abc{cur_order}", url_template="http://foo.com", + order=cur_order, ) linkifier.save() + order_to_id[cur_order] = linkifier.id linkifiers = linkifiers_for_realm(realm.id) - for index, cur_precedence in enumerate(sorted(sequence)): - self.assertEqual(linkifiers[index]["id"], cur_precedence) + for index, cur_order in enumerate(sorted(order_values)): + self.assertEqual(linkifiers[index]["id"], order_to_id[cur_order]) def test_realm_patterns_negative(self) -> None: realm = get_realm("zulip") diff --git a/zerver/tests/test_realm_linkifiers.py b/zerver/tests/test_realm_linkifiers.py index 1fe625ae4f..1a43d33707 100644 --- a/zerver/tests/test_realm_linkifiers.py +++ b/zerver/tests/test_realm_linkifiers.py @@ -1,9 +1,12 @@ import re +from typing import List +import orjson from django.core.exceptions import ValidationError from zerver.lib.test_classes import ZulipTestCase -from zerver.models import RealmFilter, url_template_validator +from zerver.lib.utils import assert_is_not_none +from zerver.models import RealmAuditLog, RealmFilter, url_template_validator class RealmFilterTest(ZulipTestCase): @@ -265,3 +268,118 @@ class RealmFilterTest(ZulipTestCase): for url in invalid_urls: with self.assertRaises(ValidationError): url_template_validator(url) + + def test_reorder_linkifiers(self) -> None: + iago = self.example_user("iago") + self.login("iago") + + def assert_linkifier_audit_logs(expected_id_order: List[int]) -> None: + """Check if the audit log created orders the linkifiers correctly""" + extra_data = orjson.loads( + assert_is_not_none( + RealmAuditLog.objects.filter( + acting_user=iago, + event_type=RealmAuditLog.REALM_LINKIFIERS_REORDERED, + ) + .latest("event_time") + .extra_data + ) + ) + audit_logged_ids = [ + linkifier_dict["id"] for linkifier_dict in extra_data["realm_linkifiers"] + ] + self.assertListEqual(expected_id_order, audit_logged_ids) + + def assert_linkifier_order(expected_id_order: List[int]) -> None: + """Verify that the realm audit log created matches the expected ordering""" + result = self.client_get("/json/realm/linkifiers") + actual_id_order = [ + linkifier["id"] for linkifier in self.assert_json_success(result)["linkifiers"] + ] + self.assertListEqual(expected_id_order, actual_id_order) + + def reorder_verify_succeed(expected_id_order: List[int]) -> None: + """Send a reorder request and verify that it succeeds""" + result = self.client_patch( + "/json/realm/linkifiers", + {"ordered_linkifier_ids": orjson.dumps(expected_id_order).decode()}, + ) + self.assert_json_success(result) + + reorder_verify_succeed([]) + self.assertEqual( + RealmAuditLog.objects.filter( + realm=iago.realm, event_type=RealmAuditLog.REALM_LINKIFIERS_REORDERED + ).count(), + 0, + ) + + linkifiers = [ + { + "pattern": "1#(?P[123])", + "url_template": "https://filter.com/foo/{id}", + }, + { + "pattern": "2#(?P[123])", + "url_template": "https://filter.com/bar/{id}", + }, + { + "pattern": "3#(?P[123])", + "url_template": "https://filter.com/baz/{id}", + }, + ] + original_id_order = [] + for linkifier in linkifiers: + result = self.client_post("/json/realm/filters", linkifier) + original_id_order.append(self.assert_json_success(result)["id"]) + assert_linkifier_order(original_id_order) + self.assertListEqual([0, 1, 2], list(RealmFilter.objects.values_list("order", flat=True))) + + # The creation order orders the linkifiers by default. + # When the order values are the same, fallback to order by ID. + RealmFilter.objects.all().update(order=0) + assert_linkifier_order(original_id_order) + + # This should successfully reorder the linkifiers. + new_order = [original_id_order[2], original_id_order[1], original_id_order[0]] + reorder_verify_succeed(new_order) + assert_linkifier_audit_logs(new_order) + assert_linkifier_order(new_order) + + # After reordering, newly created linkifier is ordered at the last, and + # the other linkifiers are unchanged. + result = self.client_post( + "/json/realm/filters", {"pattern": "3#123", "url_template": "https://example.com"} + ) + new_linkifier_id = self.assert_json_success(result)["id"] + new_order = [*new_order, new_linkifier_id] + assert_linkifier_order(new_order) + + # Deleting a linkifier should preserve the order. + deleted_linkifier_id = new_order[2] + result = self.client_delete(f"/json/realm/filters/{deleted_linkifier_id}") + self.assert_json_success(result) + new_order = [*new_order[:2], new_linkifier_id] + assert_linkifier_order(new_order) + + # Extra non-existent ids are ignored. + new_order = [new_order[2], new_order[0], new_order[1]] + result = self.client_patch( + "/json/realm/linkifiers", {"ordered_linkifier_ids": [deleted_linkifier_id, *new_order]} + ) + self.assert_json_error( + result, "The ordered list must enumerate all existing linkifiers exactly once" + ) + + # Duplicated IDs are not allowed. + new_order = [*new_order, new_order[0]] + result = self.client_patch("/json/realm/linkifiers", {"ordered_linkifier_ids": new_order}) + self.assert_json_error(result, "The ordered list must not contain duplicated linkifiers") + + # Incomplete lists of linkifiers are not allowed. + result = self.client_patch( + "/json/realm/linkifiers", {"ordered_linkifier_ids": new_order[:2]} + ) + self.assert_json_error( + result, "The ordered list must enumerate all existing linkifiers exactly once" + ) diff --git a/zerver/views/realm_linkifiers.py b/zerver/views/realm_linkifiers.py index f4bda4b324..300e1d6f8c 100644 --- a/zerver/views/realm_linkifiers.py +++ b/zerver/views/realm_linkifiers.py @@ -1,8 +1,11 @@ +from typing import List + from django.core.exceptions import ValidationError from django.http import HttpRequest, HttpResponse from django.utils.translation import gettext as _ from zerver.actions.realm_linkifiers import ( + check_reorder_linkifiers, do_add_linkifier, do_remove_linkifier, do_update_linkifier, @@ -11,6 +14,7 @@ from zerver.decorator import require_realm_admin from zerver.lib.exceptions import JsonableError, ValidationFailureError from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_success +from zerver.lib.validator import check_int, check_list from zerver.models import RealmFilter, UserProfile, linkifiers_for_realm @@ -73,3 +77,14 @@ def update_linkifier( raise JsonableError(_("Linkifier not found.")) except ValidationError as e: raise ValidationFailureError(e) + + +@require_realm_admin +@has_request_variables +def reorder_linkifiers( + request: HttpRequest, + user_profile: UserProfile, + ordered_linkifier_ids: List[int] = REQ(json_validator=check_list(check_int)), +) -> HttpResponse: + check_reorder_linkifiers(user_profile.realm, ordered_linkifier_ids, acting_user=user_profile) + return json_success(request) diff --git a/zproject/urls.py b/zproject/urls.py index a2a149812b..06708eb818 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -114,6 +114,7 @@ from zerver.views.realm_linkifiers import ( create_linkifier, delete_linkifier, list_linkifiers, + reorder_linkifiers, update_linkifier, ) from zerver.views.realm_logo import delete_logo_backend, get_logo_backend, upload_logo @@ -270,7 +271,7 @@ v1_api_and_json_patterns = [ # realm/logo -> zerver.views.realm_logo rest_path("realm/logo", POST=upload_logo, DELETE=delete_logo_backend, GET=get_logo_backend), # realm/filters and realm/linkifiers -> zerver.views.realm_linkifiers - rest_path("realm/linkifiers", GET=list_linkifiers), + rest_path("realm/linkifiers", GET=list_linkifiers, PATCH=reorder_linkifiers), rest_path("realm/filters", POST=create_linkifier), rest_path("realm/filters/", DELETE=delete_linkifier, PATCH=update_linkifier), # realm/playgrounds -> zerver.views.realm_playgrounds