From 2fb67b3f324e89eaf9b27c7fd3efc0ef844a5d69 Mon Sep 17 00:00:00 2001
From: Steve Howell <showell@zulip.com>
Date: Wed, 24 Jun 2020 23:01:10 +0000
Subject: [PATCH] refactor: Extract add/remove_subscriptions_schema.

Now we are consistent about validating color/description.

Ideally we wouldn't need to validate the
`streams_raw` parameters multiple times per
request, but the outer function here changes
the error messages to explicitly reference
the "delete" and "add" request variables.

And for the situation where the user-supplied
parameters are correct, the performance penalty
for checking them twice is extremely negligible.

So it's probably fine for now to just make sure
we use the same validators in all the relevant
places.

There's probably some deeper refactor that we
can do to eliminate the whole `compose_views`
scheme.  And it's also not entirely clear to
me that we really need to support the update
endpoint.  But that's all out of the scope of
this commit.
---
 zerver/views/streams.py | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/zerver/views/streams.py b/zerver/views/streams.py
index 115af702a6..6deeb2991d 100644
--- a/zerver/views/streams.py
+++ b/zerver/views/streams.py
@@ -288,12 +288,25 @@ def list_subscriptions_backend(
 
 FuncKwargPair = Tuple[Callable[..., HttpResponse], Dict[str, Union[int, Iterable[Any]]]]
 
+add_subscriptions_schema = check_list(
+    check_dict_only(
+        required_keys=[
+            ('name', check_string)
+        ],
+        optional_keys=[
+            ('color', check_color),
+            ('description', check_capped_string(Stream.MAX_DESCRIPTION_LENGTH)),
+        ],
+    ),
+)
+
+remove_subscriptions_schema = check_list(check_string)
+
 @has_request_variables
 def update_subscriptions_backend(
         request: HttpRequest, user_profile: UserProfile,
-        delete: Iterable[str]=REQ(validator=check_list(check_string), default=[]),
-        add: Iterable[Mapping[str, Any]]=REQ(
-            validator=check_list(check_dict([('name', check_string)])), default=[]),
+        delete: Iterable[str]=REQ(validator=remove_subscriptions_schema, default=[]),
+        add: Iterable[Mapping[str, Any]]=REQ(validator=add_subscriptions_schema, default=[]),
 ) -> HttpResponse:
     if not add and not delete:
         return json_error(_('Nothing to do. Specify at least one of "add" or "delete".'))
@@ -335,7 +348,7 @@ check_principals: Validator[Union[List[str], List[int]]] = check_union(
 @has_request_variables
 def remove_subscriptions_backend(
         request: HttpRequest, user_profile: UserProfile,
-        streams_raw: Iterable[str]=REQ("subscriptions", validator=check_list(check_string)),
+        streams_raw: Iterable[str]=REQ("subscriptions", validator=remove_subscriptions_schema),
         principals: Optional[Union[List[str], List[int]]]=REQ(validator=check_principals, default=None),
 ) -> HttpResponse:
 
@@ -394,13 +407,7 @@ EMPTY_PRINCIPALS: Union[Sequence[str], Sequence[int]] = []
 @has_request_variables
 def add_subscriptions_backend(
         request: HttpRequest, user_profile: UserProfile,
-        streams_raw: Iterable[Dict[str, str]]=REQ(
-            "subscriptions", validator=check_list(check_dict_only(
-                [('name', check_string)], optional_keys=[
-                    ('color', check_color),
-                    ('description', check_capped_string(Stream.MAX_DESCRIPTION_LENGTH)),
-                ]),
-            )),
+        streams_raw: Iterable[Dict[str, str]]=REQ("subscriptions", validator=add_subscriptions_schema),
         invite_only: bool=REQ(validator=check_bool, default=False),
         stream_post_policy: int=REQ(validator=check_int_in(
             Stream.STREAM_POST_POLICY_TYPES), default=Stream.STREAM_POST_POLICY_EVERYONE),