Use dictionaries instead of tuples for linkifiers. #17596

abhijeetbodas2001 · 2021-03-12T14:44:05Z

https://chat.zulip.org/#narrow/stream/49-development-help/topic/Nullable.20element.20in.20TupleType

This adds new event type realm_linkifier which uses dictionaries instead of tuples.

Testing plan:
This PR updates the webapp frontend to handle the new-type events, but I also tested manually before making the frontend changes to verify that the old style events are still sent as before.

showell · 2021-03-12T15:25:24Z

zerver/lib/event_schema.py

+
+# This is a legacy event type to ensure backwards compatibility
+# for old clients. Newer clients should handle only the
+# "realm_linkifiers" event above.


Does realm_filter_type get used any more after your changes? We should keep it, since we still need to support old clients. But we probably won't test it in test_events. Instead, we'll probably want you to introduce some test in test_linkifiers.py or similar that simulates an old client connecting to us, and we'll want to make sure the payload adheres to this schema. I'm not exactly sure where that test should go, actually, and there might be an existing test that we just modify.

showell · 2021-03-12T20:56:18Z

This looks very clean and thorough! One quick question is where do we test with the old schema (i.e. realm_filter_type via check_realm_filters now in the backend tests?

edit - I see it now...makes sense that it's still in the event test since we're still send the event

abhijeetbodas2001 · 2021-03-12T21:22:36Z

The last push was just a change to the commit body so as to mention that we send both the old and the new event types.

zerver/lib/markdown/__init__.py

zerver/management/commands/realm_linkifiers.py

timabbott · 2021-03-15T18:25:30Z

zerver/management/commands/realm_linkifiers.py

        pattern = options["pattern"]
        if not pattern:
-            self.print_help("./manage.py", "realm_filters")
+            self.print_help("./manage.py", "realm_linkifier")


I think there's probably a prep commit you could do to make this access its own command name via sys.args[1] or something rather than hardcoding? Not sure how easy this is with Django's management command system, but that'd be a good sweep to do.

I added a prep commit which does that sweep, and yet another one before it as a small correction 😄

zerver/models.py

timabbott · 2021-03-15T18:27:58Z

zerver/models.py

-    global per_request_realm_filters_cache
-    per_request_realm_filters_cache = {}
+    global per_request_linkifiers_cache
+    per_request_linkifiers_cache = {}


Definitely don't try to do this change in this commit, but I think we can actually rename RealmFilter to Linkifier in models.py with a small Django configuration option to set the database table name to still be zerver_realmfilter.

I haven't put any of the following in this PR, but just writing my findings here...

There's a db_table meta property which we can specify for the table name. So a diff like this-

-class RealmFilter(models.Model): +class Linkifier(models.Model): """Realm-specific regular expressions to automatically linkify certain strings inside the Markdown processor. See "Custom filters" in the settings UI. """ @@ -869,6 +869,7 @@ class RealmFilter(models.Model): url_format_string: str = models.TextField(validators=[URLValidator(), filter_format_validator]) class Meta: + db_table = "zerver_realmfilter" unique_together = ("realm", "pattern")

Results in a migration like so-

operations = [ migrations.RenameModel( old_name='RealmFilter', new_name='Linkifier', ), migrations.AlterModelTable( name='linkifier', table='zerver_realmfilter', ), ]

Is that what you have in mind?

Another question I have is, why not change the database table name? I tried running migrations both ways -- specifying the db_name to be the old zerver_realmfilter and not specifying it (so that it defaults to zerver_linkifier). There seems to be no data loss in either case.

timabbott · 2021-03-15T18:29:58Z

zerver/tests/test_realm_linkifiers.py


        data["pattern"] = r"lp:(?P<id>[0-9]+)"
-        data["url_format_string"] = "https://realm.com/my_realm_filter/?sort=reverse&value=%(id)s"
+        data["url_format_string"] = "https://realm.com/my_linkifier/?sort=reverse&value=%(id)s"


Definitely don't this in the current commit, but ideally we should change these to use the IETF standard example.com as the main domain. (Maybe bugs.example.com/?sort... for this launchpad example, and similar for the ones that are Jira or the like.

I removed these specific changes from the commit. Many similar URL strings are also there in markdown_test_cases.json. Do those need to be changed as well?

timabbott · 2021-03-15T18:30:58Z

I posted a hopefully complete batch of comments on the 2nd commit (the first was merged in #17625). Thanks @abhijeetbodas2001, I'm very excited about this.

abhijeetbodas2001 · 2021-03-19T12:21:20Z

@timabbott Thanks for the review! I addressed most of your comments, except the last couple ones. I didn't understand what you mean by "don't try to do this in this commit". Do you mean it should be extracted as a prep commit here, or a commit at the end of this PR, or as a different follow-up PR all together?

showell · 2021-03-19T12:34:12Z

I would be inclined to do the change of moving things to example.com as a separate PR. I'm not sure that's exactly what Tim intended, but I think the gist of his comment is that he thinks it's a worthwhile cleanup, just not in that particular commit.

abhijeetbodas2001 · 2021-03-30T12:29:50Z

I've done some splitting of commits to try to make this more readable. The last commit is still huge (200+ lines), but a lot of it is zulip.yaml.

timabbott · 2021-04-06T01:13:34Z

I got this test failure nondeterministically when running tests in parallel with the second commit included. Haven't tried to debug.

======================================================================
ERROR: test_management_commands_show_help (zerver.tests.test_management_commands.TestCommandsCanStart) [<object object at 0x7f9afcc7aa80>] (management_command='check_redis')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.6/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib/python3.6/unittest/case.py", line 523, in subTest
    yield
  File "/home/tabbott/zulip/zerver/tests/test_management_commands.py", line 215, in test_management_commands_show_help
    call_command(command, "--help")
  File "/srv/zulip-py3-venv/lib/python3.6/site-packages/django/core/management/__init__.py", line 111, in call_command
    command = load_command_class(app_name, command_name)
  File "/srv/zulip-py3-venv/lib/python3.6/site-packages/django/core/management/__init__.py", line 37, in load_command_class
    module = import_module('%s.management.commands.%s' % (app_name, name))
  File "/srv/zulip-py3-venv/lib/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/tabbott/zulip/zerver/management/commands/check_redis.py", line 13, in <module>
    class Command(BaseCommand):
  File "/home/tabbott/zulip/zerver/management/commands/check_redis.py", line 17, in Command
    Usage: ./manage.py [--trim] {sys.argv[1]}"""
IndexError: list index out of range

timabbott · 2021-04-06T01:19:39Z

OK I merged most of the prep commits through 52a86d9, since they're independent of that issue.

timabbott · 2021-04-06T01:20:19Z

frontend_tests/node_tests/markdown.js

+    {
+        pattern: "ZGROUP_(?P<id>[0-9]{2,8}):(?P<zone>[0-9]{1,8})",
+        url_format: "https://zone_%(zone)s.zulip.net/ticket/%(id)s",
+    },


Should these examples have an ID field too?

They aren't required for these tests -- the ids are used in only the settings page, but I'll add them here too for completeness.

I also did this treatment to a couple other instances in this file which the commit touches, so that these dictionaries are complete even if we aren't testing all of the fields.

abhijeetbodas2001 · 2021-04-09T12:25:46Z

@timabbott The main commit here is ready for another review. I've addressed #17596 (comment).
I've removed the sys.argv[1] commit from this PR for now. I hadn't run into the failure above when I ran the test a couple times before pushing the commit earlier.
I don't have an idea right now about what causes it to break. I will try to investigate it a bit more thoroughly, and add that commit (after fixing it) back here once done!

abhijeetbodas2001 · 2021-04-09T12:28:20Z

Is there a way to run only a few tests parallelly, without doing the whole of test-backend (which is pretty slow on my machine 😅)?

timabbott · 2021-04-13T18:59:18Z

Is there a way to run only a few tests parallelly, without doing the whole of test-backend (which is pretty slow on my machine sweat_smile)?

@abhijeetbodas2001 yeah, you can do test-backend <suite1> <suite2> --parallel=2. It's all documented in test-backend --help. It can also sometimes be helpful to run test-backend --reverse to look for order-dependent test failures.

* This introduces a new event type `realm_linkifiers` and a new key for the initial data fetch of the same name. Newer clients will be expected to use these. * Backwards compatibility is ensured by changing neither the current event nor the /register key. The data which these hold is the same as before, but internally, it is generated by processing the `realm_linkifiers` data. We send both the old and the new event types to clients whenever the linkifiers are changed. Older clients will simply ignore the new event type, and vice versa. * The `realm/filters:GET` endpoint (which returns tuples) is currently used by none of the official Zulip clients. This commit replaces it with `realm/linkifiers:GET` which returns data in the new dictionary format. TODO: Update the `get_realm_filters` method in the API bindings, to hit this new URL instead of the old one. * This also updates the webapp frontend to use the newer events and keys.

timabbott · 2021-04-13T19:19:08Z

This is great, merged, thanks @abhijeetbodas2001! I made the following tweaks to documentation while merging:

diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md
index f8d8dcccb3..a4fe251942 100644
--- a/templates/zerver/api/changelog.md
+++ b/templates/zerver/api/changelog.md
@@ -12,13 +12,20 @@ below features are supported.
 
 **Feature level 54**
 
-* `GET /realm/filters` is replaced by [`GET /realm/linkifiers`](/api/get-linkifiers)
-  which returns the data in a cleaner, dictionary format.
-* [`GET /events`](/api/get-events): Introduced new event type `realm_linkifiers`.
-  This will replace the `realm_filters` events in the future.
-* [`POST /register`](/api/register-queue): The response now also contains a
-  `realm_linkifiers` key, which is similar to the existing `realm_filters` key,
-  but uses dictionaries instead tuples.
+* `GET /realm/filters` has been removed and replace with [`GET
+  /realm/linkifiers`](/api/get-linkifiers) which returns the data in a
+  cleaner dictionary format.
+* [`GET /events`](/api/get-events): Introduced new event type
+  `realm_linkifiers`.  The previous `realm_filters` event type is
+  still supported for backwards compatibility, but will be removed in
+  a future release.
+* [`POST /register`](/api/register-queue): The response now supports a
+  `realm_linkifiers` event type, containing the same data as the
+  legacy `realm_filters` key, with a more extensible object
+  format. The previous `realm_filters` event type is still supported
+  for backwards compatibility, but will be removed in a future
+  release. The legacy `realm_filters` key is deprecated but remains
+  available for backwards compatibility.
 
 **Feature level 53**
 
diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml
index d5c39141ee..8536ea34eb 100644
--- a/zerver/openapi/zulip.yaml
+++ b/zerver/openapi/zulip.yaml
@@ -2378,8 +2378,13 @@ paths:
                                 Processing this event is important to doing Markdown local echo
                                 correctly.
 
-                                **Changes**: New in Zulip 4.0 (feature level 54). This will eventually
-                                replace the `realm_filters` event type.
+                                **Changes**: New in Zulip 4.0 (feature level 54), replacing the
+                                previous `realm_filters` event type, which is still sent for
+                                backwards compatibility reasons.
+
+                                Clients should migrate to requesting and processing the
+                                `realm_linkifiers` event type when possible, since we plan to remove
+                                the legacy `realm_filters` logic entirely in a future release.
                               properties:
                                 id:
                                   $ref: "#/components/schemas/EventIdSchema"
@@ -2401,15 +2406,16 @@ paths:
                                         type: string
                                         description: |
                                           The string regex pattern which represents the pattern that
-                                          should be linkified on matching.
+                                          should be linkified by this linkifier.
                                       url_format:
                                         type: string
                                         description: |
-                                          The URL with which the pattern matching string should be linkified.
+                                          The URL format string to be used for linkifying matches.
+
                                       id:
                                         type: integer
                                         description: |
-                                          The ID of the realm linkifier.
+                                          The ID of the linkifier.
                               example:
                                 {
                                   "type": "realm_linkifiers",
@@ -2431,11 +2437,9 @@ paths:
                                 when the set of configured [linkifiers](/help/add-a-custom-linkifier)
                                 for the organization has changed.
 
-                                Processing this event is important to doing Markdown local echo
-                                correctly.
-
                                 **Changes**: Deprecated in Zulip 4.0 (feature level 54), replaced by
-                                the `realm_linkifiers` event type instead.
+                                the `realm_linkifiers` event type, which has a clearer name and format,
+                                instead.
                               properties:
                                 id:
                                   $ref: "#/components/schemas/EventIdSchema"
@@ -6556,8 +6560,9 @@ paths:
 
         `GET {{ api_url }}/v1/realm/linkifiers`
 
-        **Changes**: New in Zulip 4.0 (feature level 54). This replaces
-        the old `GET /realm/filters` endpoint completely.
+        **Changes**: New in Zulip 4.0 (feature level 54). On older versions,
+        a similar `GET /realm/filters` endpoint was available with each entry in
+        a `[pattern, url_format, id]` tuple format.
       responses:
         "200":
           description: Success.
@@ -6582,11 +6587,11 @@ paths:
                               type: string
                               description: |
                                 The string regex pattern which represents the pattern that
-                                should be linkified on matching.
+                                should be linkified by this linkifier.
                             url_format:
                               type: string
                               description: |
-                                The URL with which the pattern matching string should be linkified.
+                                The URL format string to be used for linkifying matches.
                             id:
                               type: integer
                               description: |
@@ -7085,7 +7090,9 @@ paths:
                           Array of objects where each object describes a single
                           [linkifier](/help/add-a-custom-linkifier).
 
-                          **Changes**: New in Zulip 4.0 (feature level 54).
+                          **Changes**: New in Zulip 4.0 (feature level 54). Clients can
+                          access these data on older server versions via the previous
+                          `realm_filters` key.
                         items:
                           type: object
                           additionalProperties: false

timabbott · 2021-04-13T19:20:01Z

@abhijeetbodas2001 I think as a natural follow-up, we should change the POST endpoint for linkifiers as well. That endpoint is only used by the webapp today, so we should be able to move it over without leaving a backwards-compatible option.

timabbott · 2021-04-13T19:23:21Z

I've also opened issues for mobile and terminal to migrate to the new format. Thanks for doing this @abhijeetbodas2001! It's very nice to be on a path to eliminating these weird tuple quirks from our API documentation.

abhijeetbodas2001 · 2021-04-15T10:12:20Z

Yay! I think that leaves just muted_topic_type events now? Is there a good enough reason to convert those too in the near future?

abhijeetbodas2001 · 2021-04-15T10:33:57Z

@abhijeetbodas2001 yeah, you can do test-backend --parallel=2. It's all documented in test-backend --help. It can also sometimes be helpful to run test-backend --reverse to look for order-dependent test failures.

Thanks. Pretty dumb of me to not look at --help before asking though 🤦‍♂️

abhijeetbodas2001 · 2021-04-15T10:46:25Z

Let me compile a list of TODOs from here-

Send a PR to python-zulip-api with the GET realm/linkifiers update. (zulip: Replace GET realm/filters with realm/linkifiers. python-zulip-api#681)
sys.argv[1] sweep for management commands Use dictionaries instead of tuples for linkifiers. #17596 (comment)
renaming the RealmFilter Django model to Linkifier Use dictionaries instead of tuples for linkifiers. #17596 (comment)
Converting realm/filters:POST to realm/linkifiers:POST. The mobile repo has code to call this enpoint, but it looks like that module isn't called from anywhere. Is it just dead code? Also, can we not rename all of these endpoint (POST, DELETE, and the upcoming PATCH)? None of the clients other than the webapp currently allow users to edit or change linkifiers correct?
Using example.com everywhere instead of other real or dummy links Use dictionaries instead of tuples for linkifiers. #17596 (comment)
Starting work on stream-specific linkifiers!

@timabbott I would like to tackle all of these. Is the priority order correct?

zulipbot added the size: XL label Mar 12, 2021

showell reviewed Mar 12, 2021

View reviewed changes

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch 2 times, most recently from 076a117 to 28ba55e Compare March 12, 2021 20:55

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch from 28ba55e to be9414d Compare March 12, 2021 21:21

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch 3 times, most recently from 271619a to ed4c245 Compare March 14, 2021 11:26

This was referenced Mar 14, 2021

Rename "realm filter" -> "linkifier" in frontend plus some minor bug fixes. #17625

Merged

Enable stream-specific linkifiers #4627

Open

timabbott reviewed Mar 15, 2021

View reviewed changes

zerver/lib/markdown/__init__.py Outdated Show resolved Hide resolved

timabbott reviewed Mar 15, 2021

View reviewed changes

zerver/management/commands/realm_linkifiers.py Outdated Show resolved Hide resolved

timabbott reviewed Mar 15, 2021

View reviewed changes

zerver/models.py Outdated Show resolved Hide resolved

timabbott reviewed Mar 15, 2021

View reviewed changes

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch 2 times, most recently from 44682b2 to 9bf7954 Compare March 19, 2021 10:37

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch 3 times, most recently from 6ade50a to 7d54b38 Compare March 28, 2021 11:17

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch 2 times, most recently from 630cf6d to f15be09 Compare March 30, 2021 12:04

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch from f15be09 to f29e588 Compare April 1, 2021 11:51

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch 6 times, most recently from fa49d09 to b50ad66 Compare April 4, 2021 13:32

timabbott reviewed Apr 6, 2021

View reviewed changes

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch from b50ad66 to 368937c Compare April 6, 2021 14:33

timabbott mentioned this pull request Apr 7, 2021

model/views: Add settings support for "playground" feature (backend) #16496

Merged

5 tasks

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch 3 times, most recently from 686c5e2 to 873c717 Compare April 9, 2021 12:15

abhijeetbodas2001 force-pushed the tuple_to_dict_linkifiers branch from 873c717 to 3c948f5 Compare April 13, 2021 11:24

timabbott force-pushed the tuple_to_dict_linkifiers branch from 3c948f5 to 3947b0c Compare April 13, 2021 19:18

timabbott merged commit 3947b0c into zulip:master Apr 13, 2021

This was referenced Apr 13, 2021

Migrate from realm_filters event format to new realm_linkifiers format zulip/zulip-mobile#4657

Closed

Migrate from realm_filters event format to new realm_linkifiers format zulip/zulip-terminal#995

Open

abhijeetbodas2001 deleted the tuple_to_dict_linkifiers branch April 15, 2021 10:07

Uh oh!

Use dictionaries instead of tuples for linkifiers. #17596

Use dictionaries instead of tuples for linkifiers. #17596

Uh oh!

Conversation

abhijeetbodas2001 commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showell commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhijeetbodas2001 commented Mar 12, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timabbott commented Mar 15, 2021

Uh oh!

abhijeetbodas2001 commented Mar 19, 2021

Uh oh!

showell commented Mar 19, 2021

Uh oh!

abhijeetbodas2001 commented Mar 30, 2021

Uh oh!

timabbott commented Apr 6, 2021

Uh oh!

timabbott commented Apr 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhijeetbodas2001 Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhijeetbodas2001 commented Apr 9, 2021

Uh oh!

abhijeetbodas2001 commented Apr 9, 2021

Uh oh!

timabbott commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timabbott commented Apr 13, 2021

Uh oh!

timabbott commented Apr 13, 2021

Uh oh!

timabbott commented Apr 13, 2021

Uh oh!

abhijeetbodas2001 commented Apr 15, 2021

Uh oh!

abhijeetbodas2001 commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhijeetbodas2001 commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

abhijeetbodas2001 commented Mar 12, 2021 •

edited

Loading

showell commented Mar 12, 2021 •

edited

Loading

abhijeetbodas2001 Apr 9, 2021 •

edited

Loading

timabbott commented Apr 13, 2021 •

edited

Loading

abhijeetbodas2001 commented Apr 15, 2021 •

edited

Loading

abhijeetbodas2001 commented Apr 15, 2021 •

edited

Loading