apply_event: Fix broken deepcopy attempt for subs.

When we were getting an apply_event call for
a subscription/add event, we were trying not to
mutate the event itself, but this clumsy code
was still mutating the actual event:

    # Avoid letting 'subscribers' entries end up in the list
    for i, sub in enumerate(event['subscriptions']):
        event['subscriptions'][i] = \
            copy.deepcopy(event['subscriptions'][i])
        del event['subscriptions'][i]['subscribers']

This is only a theoretical bug.

The only person who receives a subscription/add
event is the current user.

And it wouldn't have affected the current user,
since the apply_event was correctly updating the
state, and we wouldn't actually deliver the event
to the client (because the whole point of apply_event
is to prevent us from having to piggyback the
super-recent events on to our payload or put
them into the event queue and possibly race).

The new code just cleanly makes a copy of each
sub, if necessary, as we add them to state["subscriptions"].

And I updated the event schemas to reflect that
subscribers is always present in subscription/add
event.

Long term we should probably avoid sending subscribers
on this event when the clients don't set something
like include_subscribers.  That's a fairly complicated
fix that involves passing in flags to ClientDescriptor.
Alternatively, we could just say that our policy is
that we never send subscribers there, but we instead
use peer_add events.  See issue #17089 for more
details.

zerver/lib/event_schema.py

@@ -77,6 +77,10 @@ subscription_fields: Sequence[Tuple[str, object]] = [
     ("push_notifications", OptionalType(bool)),
     ("role", EnumType(Subscription.ROLE_TYPES)),
     ("stream_weekly_traffic", OptionalType(int)),
+    # We may try to remove subscribers from some events in
+    # the future for clients that don't want subscriber
+    # info.
+    ("subscribers", ListType(int)),
     ("wildcard_mentions_notify", OptionalType(bool)),
 ]
 
@@ -1150,11 +1154,8 @@ submessage_event = event_dict_type(
 check_submessage = make_checker(submessage_event)
 
 single_subscription_type = DictType(
+    # force vertical
     required_keys=subscription_fields,
-    optional_keys=[
-        # force vertical
-        ("subscribers", ListType(int)),
-    ],
 )
 
 subscription_add_event = event_dict_type(
@@ -1164,21 +1165,7 @@ subscription_add_event = event_dict_type(
         ("subscriptions", ListType(single_subscription_type)),
     ],
 )
-_check_subscription_add = make_checker(subscription_add_event)
-
-
-def check_subscription_add(
-    var_name: str, event: Dict[str, object], include_subscribers: bool,
-) -> None:
-    _check_subscription_add(var_name, event)
-
-    assert isinstance(event["subscriptions"], list)
-    for sub in event["subscriptions"]:
-        if include_subscribers:
-            assert "subscribers" in sub.keys()
-        else:
-            assert "subscribers" not in sub.keys()
-
+check_subscription_add = make_checker(subscription_add_event)
 
 subscription_peer_add_event = event_dict_type(
     required_keys=[

zerver/lib/events.py

@@ -703,17 +703,18 @@ def apply_event(
                     state['realm_email_auth_enabled'] = value['Email']
     elif event['type'] == "subscription":
         if event["op"] == "add":
-            if not include_subscribers:
-                # Avoid letting 'subscribers' entries end up in the list
-                for i, sub in enumerate(event['subscriptions']):
-                    event['subscriptions'][i] = copy.deepcopy(event['subscriptions'][i])
-                    del event['subscriptions'][i]['subscribers']
-
             added_stream_ids = {sub["stream_id"] for sub in event["subscriptions"]}
             was_added = lambda s: s["stream_id"] in added_stream_ids
 
+            existing_stream_ids = {sub["stream_id"] for sub in state["subscriptions"]}
+
             # add the new subscriptions
-            state['subscriptions'] += event['subscriptions']
+            for sub in event["subscriptions"]:
+                if sub["stream_id"] not in existing_stream_ids:
+                    if "subscribers" in sub and not include_subscribers:
+                        sub = copy.deepcopy(sub)
+                        del sub["subscribers"]
+                    state["subscriptions"].append(sub)
 
             # remove them from unsubscribed if they had been there
             state['unsubscribed'] = [s for s in state['unsubscribed'] if not was_added(s)]

zerver/tests/test_events.py

@@ -1868,7 +1868,7 @@ class SubscribeActionTest(BaseAction):
             action,
             event_types=["subscription"],
             include_subscribers=include_subscribers)
-        check_subscription_add('events[0]', events[0], include_subscribers)
+        check_subscription_add('events[0]', events[0])
 
         # Add another user to that totally new stream
         action = lambda: self.subscribe(self.example_user("othello"), "test_stream")
@@ -1936,7 +1936,7 @@ class SubscribeActionTest(BaseAction):
             include_subscribers=include_subscribers,
             include_streams=False,
             num_events=1)
-        check_subscription_add('events[0]', events[0], include_subscribers)
+        check_subscription_add('events[0]', events[0])
 
         action = lambda: do_change_stream_description(stream, 'new description')
         events = self.verify_action(
@@ -1976,7 +1976,7 @@ class SubscribeActionTest(BaseAction):
             include_subscribers=include_subscribers,
             num_events=2)
         check_stream_create('events[0]', events[0])
-        check_subscription_add('events[1]', events[1], include_subscribers)
+        check_subscription_add('events[1]', events[1])
 
         self.assertEqual(
             events[0]['streams'][0]['message_retention_days'],