From 268f858f39bcd04df97540555177666a97dea5a1 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 5 Oct 2022 14:55:31 -0400 Subject: [PATCH] linkifier: Support URL templates for linkifiers. 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 --- api_docs/changelog.md | 16 +++ help/add-a-custom-linkifier.md | 12 +- package.json | 1 + pnpm-lock.yaml | 7 + tools/test-js-with-node | 1 + version.py | 2 +- web/e2e-tests/realm-linkifier.test.ts | 16 +-- web/src/linkifiers.ts | 36 +++-- web/src/markdown.js | 16 +-- web/src/settings_linkifiers.js | 12 +- web/src/url-template.d.ts | 19 +++ .../settings/admin_linkifier_edit_form.hbs | 4 +- .../settings/linkifier_settings_admin.hbs | 8 +- web/tests/lib/events.js | 2 +- web/tests/linkifiers.test.js | 8 +- web/tests/markdown.test.js | 31 ++++- web/tests/markdown_parse.test.js | 27 ++-- zerver/actions/realm_linkifiers.py | 37 ++--- zerver/lib/event_schema.py | 24 +--- zerver/lib/events.py | 31 ++++- zerver/lib/home.py | 1 + zerver/lib/markdown/__init__.py | 14 +- zerver/lib/types.py | 2 +- ...42_remove_realmfilter_url_format_string.py | 18 +++ zerver/models.py | 54 +++----- zerver/openapi/curl_param_value_generators.py | 2 +- zerver/openapi/python_examples.py | 13 +- zerver/openapi/zulip.yaml | 94 +++++++++---- zerver/tests/test_audit_log.py | 10 +- zerver/tests/test_event_system.py | 35 ++++- zerver/tests/test_events.py | 46 ++++-- zerver/tests/test_markdown.py | 77 +++++----- zerver/tests/test_message_dict.py | 6 +- zerver/tests/test_realm_linkifiers.py | 131 ++++++++++-------- zerver/tornado/django_api.py | 2 + zerver/tornado/event_queue.py | 4 + zerver/tornado/views.py | 4 + zerver/views/events_register.py | 1 + zerver/views/realm_linkifiers.py | 8 +- zerver/webhooks/front/doc.md | 4 +- zerver/webhooks/stripe/doc.md | 4 +- 41 files changed, 533 insertions(+), 307 deletions(-) create mode 100644 web/src/url-template.d.ts create mode 100644 zerver/migrations/0442_remove_realmfilter_url_format_string.py diff --git a/api_docs/changelog.md b/api_docs/changelog.md index ee96913fac..a9bb5affba 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,21 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 7.0 +**Feature level 176** + +* [`POST /realm/filters`](/api/add-linkifier), [`realm/filters/`](/api/update-linkifier): + The parameter `url_format_string` is replaced by `url_template`. + The linkifiers now accept only [RFC 6570][rfc6570] compliant URL Templates. + The old URL format strings are no longer supported. +* [`GET /events`](/api/get-events), [`POST /register`](/api/register-queue): + The key `url_format_string` is replaced by `url_template` for the `realm_linkifiers` + event type. For backwards-compatibility, clients that do not support the + `linkifier_url_template` + [client capability](/api/register-queue#parameter-client_capabilities) + will get an empty list in the response of `/register` and not receive `realm_linkifiers` + events. Unconditionally, the deprecated event type `realm_filters` gives an empty list in the + response of `/register` and is no longer sent the clients otherwise. + **Feature level 175** * [`POST /register`](/api/register-queue), [`PATCH /settings`](/api/update-settings), @@ -1463,3 +1478,4 @@ No changes; feature level used for Zulip 3.0 release. [server-changelog]: https://zulip.readthedocs.io/en/latest/overview/changelog.html [release-lifecycle]: https://zulip.readthedocs.io/en/latest/overview/release-lifecycle.html +[rfc6570]: https://www.rfc-editor.org/rfc/rfc6570.html diff --git a/help/add-a-custom-linkifier.md b/help/add-a-custom-linkifier.md index 95e6d5e502..741ea5a47d 100644 --- a/help/add-a-custom-linkifier.md +++ b/help/add-a-custom-linkifier.md @@ -7,6 +7,10 @@ party issue trackers, like GitHub, Salesforce, Zendesk, and others. For instance, you can add a linkifier that automatically turns `#2468` into a link to `https://github.com/zulip/zulip/issues/2468`. +Linkifiers use [RFC 6570](https://www.rfc-editor.org/rfc/rfc6570.html) +compliant URL templates to represent the link to be generated from the +pattern. + If the pattern appears in a topic, Zulip adds an **Open** () button to the right of the topic in the message recipient bar that links to the appropriate URL. @@ -18,7 +22,7 @@ topic in the message recipient bar that links to the appropriate URL. {settings_tab|linkifier-settings} 1. Under **Add a new linkifier**, enter a **Pattern** and -**URL format string**. +**URL template**. 1. Click **Add linkifier**. @@ -31,21 +35,21 @@ This is best explained by example. Hash followed by a number of any length. * Pattern: `#(?P[0-9]+)` -* URL format string: `https://github.com/zulip/zulip/issues/%(id)s` +* URL template: `https://github.com/zulip/zulip/issues/{id}` * Original text: `#2468` * Automatically links to: `https://github.com/zulip/zulip/issues/2468` String of hexadecimal digits between 7 and 40 characters long. * Pattern: `(?P[0-9a-f]{7,40})` -* URL format string: `https://github.com/zulip/zulip/commit/%(id)s` +* URL template: `https://github.com/zulip/zulip/commit/{id}` * Original text: `abdc123` * Automatically links to: `https://github.com/zulip/zulip/commit/abcd123` Generic GitHub `org/repo#ID` format: * Pattern: `(?P[a-zA-Z0-9_-]+)/(?P[a-zA-Z0-9_-]+)#(?P[0-9]+)` -* URL format string: `https://github.com/%(org)s/%(repo)s/issues/%(id)s` +* URL template: `https://github.com/{org}/{repo}/issues/{id}` * Original text: `zulip/zulip#2468` * Automatically links to: `https://github.com/zulip/zulip/issues/2468` diff --git a/package.json b/package.json index 3da0f48833..114ab2c644 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "tippy.js": "^6.3.7", "turndown": "^7.0.0", "url-loader": "^4.1.1", + "url-template": "2.0.8", "webfonts-loader": "^8.0.0", "webpack": "^5.61.0", "webpack-bundle-tracker": "^1.2.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8277e864c1..baabcb4cd8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -229,6 +229,9 @@ dependencies: url-loader: specifier: ^4.1.1 version: 4.1.1(webpack@5.77.0) + url-template: + specifier: 2.0.8 + version: 2.0.8 webfonts-loader: specifier: ^8.0.0 version: 8.0.1 @@ -11640,6 +11643,10 @@ packages: requires-port: 1.0.0 dev: true + /url-template@2.0.8: + resolution: {integrity: sha512-XdVKMF4SJ0nP/O7XIPB0JwAEuT9lDIYnNsK8yGVe43y0AWoKeJNdv3ZNWh7ksJ6KqQFjOO6ox/VEitLnaVNufw==} + dev: false + /util-deprecate@1.0.2: resolution: {integrity: sha512-EPD5q1uXyFxJpCrLnCc1nHnq3gOa6DZBocAIiI2TaSCA7VCJ1UJDMagCzIkXNsUYfD1daK//LTEQ8xiIbrHtcw==} diff --git a/tools/test-js-with-node b/tools/test-js-with-node index b6023917c7..201db2b752 100755 --- a/tools/test-js-with-node +++ b/tools/test-js-with-node @@ -210,6 +210,7 @@ EXEMPT_FILES = make_set( "web/src/unread_ui.js", "web/src/upload.js", "web/src/upload_widget.ts", + "web/src/url-template.d.ts", "web/src/user_group_create.js", "web/src/user_group_create_members.js", "web/src/user_group_create_members_data.js", diff --git a/version.py b/version.py index 7f31ba9d0c..98359c529d 100644 --- a/version.py +++ b/version.py @@ -48,4 +48,4 @@ API_FEATURE_LEVEL = 175 # historical commits sharing the same major version, in which case a # minor version bump suffices. -PROVISION_VERSION = (233, 3) +PROVISION_VERSION = (234, 0) diff --git a/web/e2e-tests/realm-linkifier.test.ts b/web/e2e-tests/realm-linkifier.test.ts index 5c7951cf0a..3d11f20357 100644 --- a/web/e2e-tests/realm-linkifier.test.ts +++ b/web/e2e-tests/realm-linkifier.test.ts @@ -8,7 +8,7 @@ async function test_add_linkifier(page: Page): Promise { await page.waitForSelector(".admin-linkifier-form", {visible: true}); await common.fill_form(page, "form.admin-linkifier-form", { pattern: "#(?P[0-9]+)", - url_format_string: "https://trac.example.com/ticket/%(id)s", + url_template: "https://trac.example.com/ticket/{id}", }); await page.click("form.admin-linkifier-form button.button"); @@ -30,7 +30,7 @@ async function test_add_linkifier(page: Page): Promise { page, ".linkifier_row span.linkifier_url_format_string", ), - "https://trac.example.com/ticket/%(id)s", + "https://trac.example.com/ticket/{id}", ); } @@ -46,7 +46,7 @@ async function test_add_invalid_linkifier_pattern(page: Page): Promise { await page.waitForSelector(".admin-linkifier-form", {visible: true}); await common.fill_form(page, "form.admin-linkifier-form", { pattern: "(foo", - url_format_string: "https://trac.example.com/ticket/%(id)s", + url_template: "https://trac.example.com/ticket/{id}", }); await page.click("form.admin-linkifier-form button.button"); @@ -62,7 +62,7 @@ async function test_edit_linkifier(page: Page): Promise { await common.wait_for_micromodal_to_open(page); await common.fill_form(page, "form.linkifier-edit-form", { pattern: "(?P[0-9a-f]{40})", - url_format_string: "https://trac.example.com/commit/%(num)s", + url_template: "https://trac.example.com/commit/{num}", }); await page.click(".dialog_submit_button"); @@ -78,7 +78,7 @@ async function test_edit_linkifier(page: Page): Promise { page, ".linkifier_row span.linkifier_url_format_string", ), - "https://trac.example.com/commit/%(num)s", + "https://trac.example.com/commit/{num}", ); } @@ -87,7 +87,7 @@ async function test_edit_invalid_linkifier(page: Page): Promise { await common.wait_for_micromodal_to_open(page); await common.fill_form(page, "form.linkifier-edit-form", { pattern: "#(?Pd????)", - url_format_string: "????", + url_template: "{id", }); await page.click(".dialog_submit_button"); @@ -108,7 +108,7 @@ async function test_edit_invalid_linkifier(page: Page): Promise { page, edit_linkifier_format_status_selector, ); - assert.strictEqual(edit_linkifier_format_status, "Failed: Enter a valid URL."); + assert.strictEqual(edit_linkifier_format_status, "Failed: Invalid URL template."); await page.click(".dialog_cancel_button"); await page.waitForSelector("#dialog_widget_modal", {hidden: true}); @@ -123,7 +123,7 @@ async function test_edit_invalid_linkifier(page: Page): Promise { page, ".linkifier_row span.linkifier_url_format_string", ), - "https://trac.example.com/commit/%(num)s", + "https://trac.example.com/commit/{num}", ); } diff --git a/web/src/linkifiers.ts b/web/src/linkifiers.ts index a9f1b5473d..1921c532fa 100644 --- a/web/src/linkifiers.ts +++ b/web/src/linkifiers.ts @@ -1,29 +1,39 @@ +import url_template_lib from "url-template"; + import * as blueslip from "./blueslip"; -const linkifier_map = new Map(); // regex -> url +type LinkifierMap = Map< + RegExp, + {url_template: url_template_lib.Template; group_number_to_name: Record} +>; +const linkifier_map: LinkifierMap = new Map(); type Linkifier = { pattern: string; - url_format: string; + url_template: string; id: number; }; -export function get_linkifier_map(): Map { +export function get_linkifier_map(): LinkifierMap { return linkifier_map; } -function python_to_js_linkifier(pattern: string, url: string): [RegExp | null, string] { +function python_to_js_linkifier( + pattern: string, + url: string, +): [RegExp | null, url_template_lib.Template, Record] { // Converts a python named-group regex to a javascript-compatible numbered // group regex... with a regex! const named_group_re = /\(?P<([^>]+?)>/g; let match = named_group_re.exec(pattern); let current_group = 1; + const group_number_to_name: Record = {}; while (match) { const name = match[1]; // Replace named group with regular matching group pattern = pattern.replace("(?P<" + name + ">", "("); - // Replace named reference in URL to numbered reference - url = url.replace("%(" + name + ")s", `\\${current_group}`); + // Map numbered reference to named reference for template expansion + group_number_to_name[current_group] = name; // Reset the RegExp state named_group_re.lastIndex = 0; @@ -73,20 +83,28 @@ function python_to_js_linkifier(pattern: string, url: string): [RegExp | null, s throw error; } } - return [final_regex, url]; + const url_template = url_template_lib.parse(url); + blueslip.info(`Linkifier info ${String(final_regex)} ${url}`, group_number_to_name); + return [final_regex, url_template, group_number_to_name]; } export function update_linkifier_rules(linkifiers: Linkifier[]): void { linkifier_map.clear(); for (const linkifier of linkifiers) { - const [regex, final_url] = python_to_js_linkifier(linkifier.pattern, linkifier.url_format); + const [regex, url_template, group_number_to_name] = python_to_js_linkifier( + linkifier.pattern, + linkifier.url_template, + ); if (!regex) { // Skip any linkifiers that could not be converted continue; } - linkifier_map.set(regex, final_url); + linkifier_map.set(regex, { + url_template, + group_number_to_name, + }); } } diff --git a/web/src/markdown.js b/web/src/markdown.js index 8651bc75ff..57315062ec 100644 --- a/web/src/markdown.js +++ b/web/src/markdown.js @@ -294,19 +294,19 @@ export function get_topic_links({topic, get_linkifier_map}) { // The lower the precedence is, the more prioritized the pattern is. let precedence = 0; - for (const [pattern, url] of get_linkifier_map().entries()) { + for (const [pattern, {url_template, group_number_to_name}] of get_linkifier_map().entries()) { let match; while ((match = pattern.exec(topic)) !== null) { - let link_url = url; const matched_groups = match.slice(1); let i = 0; + const template_context = {}; while (i < matched_groups.length) { const matched_group = matched_groups[i]; const current_group = i + 1; - const back_ref = "\\" + current_group; - link_url = link_url.replace(back_ref, matched_group); + template_context[group_number_to_name[current_group]] = matched_group; i += 1; } + const link_url = url_template.expand(template_context); // We store the starting index as well, to sort the order of occurrence of the links // in the topic, similar to the logic implemented in zerver/lib/markdown/__init__.py links.push({url: link_url, text: match[0], index: match.index, precedence}); @@ -410,17 +410,17 @@ function handleEmoji({emoji_name, get_realm_emoji_url, get_emoji_codepoint}) { } function handleLinkifier({pattern, matches, get_linkifier_map}) { - let url = get_linkifier_map().get(pattern); + const {url_template, group_number_to_name} = get_linkifier_map().get(pattern); let current_group = 1; + const template_context = {}; for (const match of matches) { - const back_ref = "\\" + current_group; - url = url.replace(back_ref, match); + template_context[group_number_to_name[current_group]] = match; current_group += 1; } - return url; + return url_template.expand(template_context); } function handleTimestamp(time) { diff --git a/web/src/settings_linkifiers.js b/web/src/settings_linkifiers.js index 97b7d39e24..2f46d8a6e5 100644 --- a/web/src/settings_linkifiers.js +++ b/web/src/settings_linkifiers.js @@ -42,7 +42,7 @@ function sort_pattern(a, b) { } function sort_url(a, b) { - return compare_values(a.url_format, b.url_format); + return compare_values(a.url_template, b.url_template); } function open_linkifier_edit_form(linkifier_id) { @@ -51,7 +51,7 @@ function open_linkifier_edit_form(linkifier_id) { const html_body = render_admin_linkifier_edit_form({ linkifier_id, pattern: linkifier.pattern, - url_format_string: linkifier.url_format, + url_format_string: linkifier.url_template, }); function submit_linkifier_form() { @@ -118,8 +118,8 @@ function handle_linkifier_api_error(xhr, pattern_status, format_status, linkifie xhr.responseText = JSON.stringify({msg: errors.pattern}); ui_report.error($t_html({defaultMessage: "Failed"}), xhr, pattern_status); } - if (errors.url_format_string !== undefined) { - xhr.responseText = JSON.stringify({msg: errors.url_format_string}); + if (errors.url_template !== undefined) { + xhr.responseText = JSON.stringify({msg: errors.url_template}); ui_report.error($t_html({defaultMessage: "Failed"}), xhr, format_status); } if (errors.__all__ !== undefined) { @@ -140,7 +140,7 @@ export function populate_linkifiers(linkifiers_data) { return render_admin_linkifier_list({ linkifier: { pattern: linkifier.pattern, - url_format_string: linkifier.url_format, + url_format_string: linkifier.url_template, id: linkifier.id, }, can_modify: page_params.is_admin, @@ -151,7 +151,7 @@ export function populate_linkifiers(linkifiers_data) { predicate(item, value) { return ( item.pattern.toLowerCase().includes(value) || - item.url_format.toLowerCase().includes(value) + item.url_template.toLowerCase().includes(value) ); }, onupdate() { diff --git a/web/src/url-template.d.ts b/web/src/url-template.d.ts new file mode 100644 index 0000000000..a7d7e3180e --- /dev/null +++ b/web/src/url-template.d.ts @@ -0,0 +1,19 @@ +// We need to use an older version of url-template because our test suite setup does not +// support testing with ESM-only modules. Unfortunately, the older version also happens to +// not have type definitions, so we maintain our own copy of it. This is adapted from: +// https://github.com/bramstein/url-template/blob/a8e204a92de3168a56ef2e528ae4d841287636fd/lib/url-template.d.ts + +declare module "url-template" { + export type PrimitiveValue = string | number | boolean | null; + + export interface Template { + expand( + context: Record< + string, + PrimitiveValue | PrimitiveValue[] | Record + >, + ): string; + } + + export function parse(template: string): Template; +} diff --git a/web/templates/settings/admin_linkifier_edit_form.hbs b/web/templates/settings/admin_linkifier_edit_form.hbs index edb7e7b710..18b856423a 100644 --- a/web/templates/settings/admin_linkifier_edit_form.hbs +++ b/web/templates/settings/admin_linkifier_edit_form.hbs @@ -6,8 +6,8 @@
- - + +
diff --git a/web/templates/settings/linkifier_settings_admin.hbs b/web/templates/settings/linkifier_settings_admin.hbs index 604ea64ed1..e3f17c78dc 100644 --- a/web/templates/settings/linkifier_settings_admin.hbs +++ b/web/templates/settings/linkifier_settings_admin.hbs @@ -18,7 +18,7 @@ {{t "Pattern" }}: #(?P<id>[0-9]+)
  • - {{t "URL format string" }}: https://github.com/zulip/zulip/issues/%(id)s + {{t "URL template" }}: https://github.com/zulip/zulip/issues/{id}

    • @@ -44,8 +44,8 @@

    - - + +