Move endpoints to use stream_id instead of stream_name in their URLs

- Change `stream_name` into `stream_id` on some API endpoints that use
`stream_name` in their URLs to prevent confusion of `views` selection.

For example:
If the stream name is "foo/members", the URL would be trigger
"^streams/(?P<stream_name>.*)/members$" and it would be confusing because
we intend to use the endpoint with "^streams/(?P<stream_name>.*)$" regex.

All stream-related endpoints now use stream id instead of stream name,
except for a single endpoint that lets you convert stream names to stream ids.

See https://github.com/zulip/zulip/issues/2930#issuecomment-269576231

- Add `get_stream_id()` method to Zulip API client, and change
`get_subscribers()` method to comply with the new stream API
(replace `stream_name` with `stream_id`).

Fixes #2930.

api/zulip/__init__.py

@@ -641,13 +641,30 @@ class Client(object):
             request=request,
         )
 
+    def get_stream_id(self, stream):
+        # type: (str) -> Dict[str, Any]
+        '''
+            Example usage: client.get_stream_id('devel')
+        '''
+        stream_encoded = urllib.parse.quote(stream, safe='')
+        url = 'get_stream_id?stream=%s' % (stream_encoded,)
+        return self.call_endpoint(
+            url=url,
+            method='GET',
+            request=None,
+        )
+
     def get_subscribers(self, **request):
         # type: (**Any) -> Dict[str, Any]
         '''
             Example usage: client.get_subscribers(stream='devel')
         '''
-        stream = urllib.parse.quote(request['stream'], safe='')
+        request_stream_id = self.get_stream_id(request['stream'])
-        url = 'streams/%s/members' % (stream,)
+        try:
+            stream_id = request_stream_id['stream_id']
+        except KeyError:
+            return request_stream_id
+        url = 'streams/%d/members' % (stream_id,)
         return self.call_endpoint(
             url=url,
             method='GET',

static/js/admin.js

@@ -778,8 +778,10 @@ function _setup_page() {
         }
         $("#deactivation_stream_modal").modal("hide");
         $(".active_stream_row button").prop("disabled", true).text("Working…");
+        var stream_name = $(".active_stream_row").find('.stream_name').text();
+        var stream_id = stream_data.get_sub(stream_name).stream_id;
         channel.del({
-            url: '/json/streams/' + encodeURIComponent($(".active_stream_row").find('.stream_name').text()),
+            url: '/json/streams/' + stream_id,
             error: function (xhr) {
                 if (xhr.status.toString().charAt(0) === "4") {
                     $(".active_stream_row button").closest("td").html(

static/js/subs.js

@@ -317,7 +317,7 @@ function show_subscription_settings(sub_row) {
     loading.make_indicator(indicator_elem);
 
     channel.get({
-        url: "/json/streams/" + encodeURIComponent(sub.name) + "/members",
+        url: "/json/streams/" + stream_id + "/members",
         idempotent: true,
         success: function (data) {
             loading.destroy_indicator(indicator_elem);
@@ -1204,15 +1204,13 @@ $(function () {
         e.preventDefault();
         var sub_settings = $(e.target).closest('.subscription_settings');
         var stream_id = $(e.target).closest(".subscription_settings").attr("data-stream-id");
-        var sub = stream_data.get_sub_by_id(stream_id);
         var new_name_box = sub_settings.find('input[name="new-name"]');
         var new_name = $.trim(new_name_box.val());
 
         $("#subscriptions-status").hide();
 
         channel.patch({
-            // Stream names might contain unsafe characters so we must encode it first.
+            url: "/json/streams/" + stream_id,
-            url: "/json/streams/" + encodeURIComponent(sub.name),
             data: {new_name: JSON.stringify(new_name)},
             success: function () {
                 new_name_box.val('');
@@ -1230,13 +1228,13 @@ $(function () {
         e.preventDefault();
         var sub_settings = $(e.target).closest('.subscription_settings');
         var stream_name = get_stream_name(sub_settings);
+        var stream_id = stream_data.get_sub(stream_name).stream_id;
         var description = sub_settings.find('input[name="description"]').val();
 
         $('#subscriptions-status').hide();
 
         channel.patch({
-            // Stream names might contain unsafe characters so we must encode it first.
+            url: '/json/streams/' + stream_id,
-            url: '/json/streams/' + encodeURIComponent(stream_name),
             data: {
                 description: JSON.stringify(description),
             },
@@ -1290,7 +1288,7 @@ $(function () {
         var data = {stream_name: sub.name, is_private: is_private};
 
         channel.patch({
-            url: "/json/streams/" + encodeURIComponent(sub.name),
+            url: "/json/streams/" + stream_id,
             data: data,
             success: function () {
                 sub = stream_data.get_sub_by_id(stream_id);

zerver/tests/test_signup.py

@@ -75,12 +75,13 @@ class PublicURLTest(ZulipTestCase):
         # FIXME: We should also test the Tornado URLs -- this codepath
         # can't do so because this Django test mechanism doesn't go
         # through Tornado.
+        denmark_stream_id = Stream.objects.get(name='Denmark').id
         get_urls = {200: ["/accounts/home/", "/accounts/login/"
                           "/en/accounts/home/", "/ru/accounts/home/",
                           "/en/accounts/login/", "/ru/accounts/login/",
                           "/help/"],
                     302: ["/", "/en/", "/ru/"],
-                    401: ["/json/streams/Denmark/members",
+                    401: ["/json/streams/%d/members" % (denmark_stream_id,),
                           "/api/v1/users/me/subscriptions",
                           "/api/v1/messages",
                           "/json/messages",

zerver/tests/test_subs.py

@@ -120,7 +120,8 @@ class StreamAdminTest(ZulipTestCase):
             'stream_name': ujson.dumps('private_stream'),
             'is_private': ujson.dumps(False)
         }
-        result = self.client_patch("/json/streams/private_stream", params)
+        stream_id = Stream.objects.get(realm=user_profile.realm, name='private_stream').id
+        result = self.client_patch("/json/streams/%d" % (stream_id,), params)
         self.assert_json_error(result, 'You are not invited to this stream.')
 
         self.subscribe_to_stream(email, 'private_stream')
@@ -130,7 +131,7 @@ class StreamAdminTest(ZulipTestCase):
             'stream_name': ujson.dumps('private_stream'),
             'is_private': ujson.dumps(False)
         }
-        result = self.client_patch("/json/streams/private_stream", params)
+        result = self.client_patch("/json/streams/%d" % (stream_id,), params)
         self.assert_json_success(result)
 
         realm = user_profile.realm
@@ -150,7 +151,8 @@ class StreamAdminTest(ZulipTestCase):
             'stream_name': ujson.dumps('public_stream'),
             'is_private': ujson.dumps(True)
         }
-        result = self.client_patch("/json/streams/public_stream", params)
+        stream_id = Stream.objects.get(realm=user_profile.realm, name='public_stream').id
+        result = self.client_patch("/json/streams/%d" % (stream_id,), params)
         self.assert_json_success(result)
         stream = Stream.objects.get(name='public_stream', realm=realm)
         self.assertTrue(stream.invite_only)
@@ -164,7 +166,7 @@ class StreamAdminTest(ZulipTestCase):
         self.subscribe_to_stream(user_profile.email, stream.name)
         do_change_is_admin(user_profile, True)
 
-        result = self.client_delete('/json/streams/new_stream')
+        result = self.client_delete('/json/streams/%d' % (stream.id,))
         self.assert_json_success(result)
         subscription_exists = Subscription.objects.filter(
             user_profile=user_profile,
@@ -182,8 +184,8 @@ class StreamAdminTest(ZulipTestCase):
         self.make_stream('new_stream')
         do_change_is_admin(user_profile, True)
 
-        result = self.client_delete('/json/streams/unknown_stream')
+        result = self.client_delete('/json/streams/999999999')
-        self.assert_json_error(result, 'No such stream name')
+        self.assert_json_error(result, u'Invalid stream id')
 
     def test_deactivate_stream_backend_requires_realm_admin(self):
         # type: () -> None
@@ -191,7 +193,8 @@ class StreamAdminTest(ZulipTestCase):
         self.login(email)
         self.subscribe_to_stream(email, 'new_stream')
 
-        result = self.client_delete('/json/streams/new_stream')
+        stream_id = Stream.objects.get(name='new_stream').id
+        result = self.client_delete('/json/streams/%d' % (stream_id,))
         self.assert_json_error(result, 'Must be a realm administrator')
 
     def test_private_stream_live_updates(self):
@@ -208,7 +211,8 @@ class StreamAdminTest(ZulipTestCase):
 
         events = [] # type: List[Dict[str, Any]]
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/private_stream',
+            stream_id = Stream.objects.get(name='private_stream').id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'description': ujson.dumps('Test description')})
         self.assert_json_success(result)
 
@@ -222,7 +226,8 @@ class StreamAdminTest(ZulipTestCase):
 
         events = []
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/private_stream',
+            stream_id = Stream.objects.get(name='private_stream').id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'new_name': ujson.dumps('whatever')})
         self.assert_json_success(result)
 
@@ -242,7 +247,8 @@ class StreamAdminTest(ZulipTestCase):
 
         events = [] # type: List[Dict[str, Any]]
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/stream_name1',
+            stream_id = Stream.objects.get(name='stream_name1').id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'new_name': ujson.dumps('stream_name2')})
         self.assert_json_success(result)
 
@@ -270,7 +276,8 @@ class StreamAdminTest(ZulipTestCase):
         # Test case to handle unicode stream name change
         # *NOTE: Here Encoding is needed when Unicode string is passed as an argument*
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/stream_name2',
+            stream_id = stream_name2_exists.id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'new_name': ujson.dumps(u'नया नाम'.encode('utf-8'))})
         self.assert_json_success(result)
         # While querying, system can handle unicode strings.
@@ -281,7 +288,8 @@ class StreamAdminTest(ZulipTestCase):
         # NOTE: Unicode string being part of URL is handled cleanly
         # by client_patch call, encoding of URL is not needed.
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/नया नाम',
+            stream_id = stream_name_uni_exists.id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'new_name': ujson.dumps(u'नाम में क्या रक्खा हे'.encode('utf-8'))})
         self.assert_json_success(result)
         # While querying, system can handle unicode strings.
@@ -292,7 +300,8 @@ class StreamAdminTest(ZulipTestCase):
 
         # Test case to change name from one language to other.
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/नाम में क्या रक्खा हे',
+            stream_id = stream_name_new_uni_exists.id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'new_name': ujson.dumps(u'français'.encode('utf-8'))})
         self.assert_json_success(result)
         stream_name_fr_exists = get_stream(u'français', realm)
@@ -300,7 +309,8 @@ class StreamAdminTest(ZulipTestCase):
 
         # Test case to change name to mixed language name.
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/français',
+            stream_id = stream_name_fr_exists.id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'new_name': ujson.dumps(u'français name'.encode('utf-8'))})
         self.assert_json_success(result)
         stream_name_mixed_exists = get_stream(u'français name', realm)
@@ -312,7 +322,8 @@ class StreamAdminTest(ZulipTestCase):
         self.login(email)
         self.make_stream('stream_name1')
 
-        result = self.client_patch('/json/streams/stream_name1',
+        stream_id = Stream.objects.get(name='stream_name1').id
+        result = self.client_patch('/json/streams/%d' % (stream_id,),
                                    {'new_name': ujson.dumps('stream_name2')})
         self.assert_json_error(result, 'Must be a realm administrator')
 
@@ -327,7 +338,8 @@ class StreamAdminTest(ZulipTestCase):
 
         events = [] # type: List[Dict[str, Any]]
         with tornado_redirected_to_list(events):
-            result = self.client_patch('/json/streams/stream_name1',
+            stream_id = Stream.objects.get(realm=realm, name='stream_name1').id
+            result = self.client_patch('/json/streams/%d' % (stream_id,),
                                        {'description': ujson.dumps('Test description')})
         self.assert_json_success(result)
 
@@ -362,7 +374,8 @@ class StreamAdminTest(ZulipTestCase):
         self.subscribe_to_stream(email, 'stream_name1')
         do_change_is_admin(user_profile, False)
 
-        result = self.client_patch('/json/streams/stream_name1',
+        stream_id = Stream.objects.get(realm=user_profile.realm, name='stream_name1').id
+        result = self.client_patch('/json/streams/%d' % (stream_id,),
                                    {'description': ujson.dumps('Test description')})
         self.assert_json_error(result, 'Must be a realm administrator')
 
@@ -392,10 +405,11 @@ class StreamAdminTest(ZulipTestCase):
         """
         active_name = stream.name
         realm = stream.realm
+        stream_id = stream.id
 
         events = [] # type: List[Dict[str, Any]]
         with tornado_redirected_to_list(events):
-            result = self.client_delete('/json/streams/' + active_name)
+            result = self.client_delete('/json/streams/' + str(stream_id))
         self.assert_json_success(result)
 
         deletion_events = [e['event'] for e in events if e['event']['type'] == 'subscription']
@@ -467,7 +481,7 @@ class StreamAdminTest(ZulipTestCase):
         priv_stream = self.set_up_stream_for_deletion(
             "privstream", subscribed=False, invite_only=True)
 
-        result = self.client_delete('/json/streams/' + priv_stream.name)
+        result = self.client_delete('/json/streams/' + str(priv_stream.id))
         self.assert_json_error(
             result, "Cannot administer invite-only streams this way")
 
@@ -2036,7 +2050,8 @@ class InviteOnlyStreamTest(ZulipTestCase):
         self.assertEqual(json["already_subscribed"], {})
 
         # Make sure both users are subscribed to this stream
-        result = self.client_get("/api/v1/streams/%s/members" % (stream_name,),
+        stream_id = Stream.objects.get(name=stream_name).id
+        result = self.client_get("/api/v1/streams/%d/members" % (stream_id,),
                                  **self.api_auth(email))
         self.assert_json_success(result)
         json = ujson.loads(result.content)
@@ -2068,16 +2083,17 @@ class GetSubscribersTest(ZulipTestCase):
                 stream_name, realm)]
         self.assertEqual(sorted(result["subscribers"]), sorted(true_subscribers))
 
-    def make_subscriber_request(self, stream_name, email=None):
+    def make_subscriber_request(self, stream_id, email=None):
-        # type: (Text, Optional[str]) -> HttpResponse
+        # type: (int, Optional[str]) -> HttpResponse
         if email is None:
             email = self.email
-        return self.client_get("/api/v1/streams/%s/members" % (stream_name,),
+        return self.client_get("/api/v1/streams/%d/members" % (stream_id,),
                                **self.api_auth(email))
 
     def make_successful_subscriber_request(self, stream_name):
         # type: (Text) -> None
-        result = self.make_subscriber_request(stream_name)
+        stream_id = Stream.objects.get(name=stream_name).id
+        result = self.make_subscriber_request(stream_id)
         self.assert_json_success(result)
         self.check_well_formed_result(ujson.loads(result.content),
                                       stream_name, self.user_profile.realm)
@@ -2216,9 +2232,9 @@ class GetSubscribersTest(ZulipTestCase):
         """
         json_get_subscribers also returns the list of subscribers for a stream.
         """
-        stream_name = "unknown_stream"
+        stream_id = 99999999
-        result = self.client_get("/json/streams/%s/members" % (stream_name,))
+        result = self.client_get("/json/streams/%d/members" % (stream_id,))
-        self.assert_json_error(result, "Stream does not exist: %s" % (stream_name,))
+        self.assert_json_error(result, u'Invalid stream id')
 
     def test_json_get_subscribers(self):
         # type: () -> None
@@ -2227,8 +2243,9 @@ class GetSubscribersTest(ZulipTestCase):
         also returns the list of subscribers for a stream.
         """
         stream_name = gather_subscriptions(self.user_profile)[0][0]['name']
+        stream_id = Stream.objects.get(realm=self.user_profile.realm, name=stream_name).id
         expected_subscribers = gather_subscriptions(self.user_profile)[0][0]['subscribers']
-        result = self.client_get("/json/streams/%s/members" % (stream_name,))
+        result = self.client_get("/json/streams/%d/members" % (stream_id,))
         self.assert_json_success(result)
         result_dict = ujson.loads(result.content)
         self.assertIn('subscribers', result_dict)
@@ -2251,6 +2268,7 @@ class GetSubscribersTest(ZulipTestCase):
         other_email = "othello@zulip.com"
 
         # Try to fetch the subscriber list as a non-member.
-        result = self.make_subscriber_request(stream_name, email=other_email)
+        stream_id = Stream.objects.get(name=stream_name).id
+        result = self.make_subscriber_request(stream_id, email=other_email)
         self.assert_json_error(result,
                                "Unable to retrieve subscribers for invite-only stream")

zerver/views/streams.py

@@ -20,7 +20,7 @@ from zerver.lib.actions import bulk_remove_subscriptions, \
 from zerver.lib.response import json_success, json_error, json_response
 from zerver.lib.validator import check_string, check_list, check_dict, \
     check_bool, check_variable_type
-from zerver.models import UserProfile, Stream, Subscription, \
+from zerver.models import UserProfile, Stream, Realm, Subscription, \
     Recipient, get_recipient, get_stream, bulk_get_streams, \
     bulk_get_recipients, valid_stream_name, get_active_user_dicts_in_realm
 
@@ -132,11 +132,9 @@ def principal_to_user_profile(agent, principal):
     return principal_user_profile
 
 @require_realm_admin
-def deactivate_stream_backend(request, user_profile, stream_name):
+def deactivate_stream_backend(request, user_profile, stream_id):
-    # type: (HttpRequest, UserProfile, Text) -> HttpResponse
+    # type: (HttpRequest, UserProfile, int) -> HttpResponse
-    target = get_stream(stream_name, user_profile.realm)
+    target = get_and_validate_stream_by_id(stream_id, user_profile.realm)
-    if not target:
-        return json_error(_('No such stream name'))
 
     if target.invite_only and not subscribed_to_stream(user_profile, target):
         return json_error(_('Cannot administer invite-only streams this way'))
@@ -160,11 +158,14 @@ def remove_default_stream(request, user_profile, stream_name=REQ()):
 
 @require_realm_admin
 @has_request_variables
-def update_stream_backend(request, user_profile, stream_name,
+def update_stream_backend(request, user_profile, stream_id,
                           description=REQ(validator=check_string, default=None),
                           is_private=REQ(validator=check_bool, default=None),
                           new_name=REQ(validator=check_string, default=None)):
-    # type: (HttpRequest, UserProfile, Text, Optional[Text], Optional[bool], Optional[Text]) -> HttpResponse
+    # type: (HttpRequest, UserProfile, int, Optional[Text], Optional[bool], Optional[Text]) -> HttpResponse
+    stream = get_and_validate_stream_by_id(stream_id, user_profile.realm)
+    stream_name = stream.name
+
     if description is not None:
         do_change_stream_description(user_profile.realm, stream_name, description)
     if stream_name is not None and new_name is not None:
@@ -406,12 +407,10 @@ def add_subscriptions_backend(request, user_profile,
     return json_success(result)
 
 @has_request_variables
-def get_subscribers_backend(request, user_profile, stream_name=REQ('stream')):
+def get_subscribers_backend(request, user_profile,
-    # type: (HttpRequest, UserProfile, Text) -> HttpResponse
+                            stream_id=REQ('stream', converter=to_non_negative_int)):
-    stream = get_stream(stream_name, user_profile.realm)
+    # type: (HttpRequest, UserProfile, int) -> HttpResponse
-    if stream is None:
+    stream = get_and_validate_stream_by_id(stream_id, user_profile.realm)
-        raise JsonableError(_("Stream does not exist: %s") % (stream_name,))
-
     subscribers = get_subscriber_emails(stream, user_profile)
 
     return json_success({'subscribers': subscribers})
@@ -436,11 +435,7 @@ def get_streams_backend(request, user_profile,
 def get_topics_backend(request, user_profile,
                        stream_id=REQ(converter=to_non_negative_int)):
     # type: (HttpRequest, UserProfile, int) -> HttpResponse
-
+    stream = get_and_validate_stream_by_id(stream_id, user_profile.realm)
-    try:
-        stream = Stream.objects.get(pk=stream_id)
-    except Stream.DoesNotExist:
-        return json_error(_("Invalid stream id"))
 
     if stream.realm_id != user_profile.realm_id:
         return json_error(_("Invalid stream id"))
@@ -468,13 +463,20 @@ def get_topics_backend(request, user_profile,
 def json_stream_exists(request, user_profile, stream=REQ(),
                        autosubscribe=REQ(default=False)):
     # type: (HttpRequest, UserProfile, Text, bool) -> HttpResponse
-    return stream_exists_backend(request, user_profile, stream, autosubscribe)
+    if not valid_stream_name(stream):
-
-def stream_exists_backend(request, user_profile, stream_name, autosubscribe):
-    # type: (HttpRequest, UserProfile, Text, bool) -> HttpResponse
-    if not valid_stream_name(stream_name):
         return json_error(_("Invalid characters in stream name"))
-    stream = get_stream(stream_name, user_profile.realm)
+    try:
+        stream_id = Stream.objects.get(realm=user_profile.realm, name=stream).id
+    except Stream.DoesNotExist:
+        stream_id = None
+    return stream_exists_backend(request, user_profile, stream_id, autosubscribe)
+
+def stream_exists_backend(request, user_profile, stream_id, autosubscribe):
+    # type: (HttpRequest, UserProfile, int, bool) -> HttpResponse
+    try:
+        stream = get_and_validate_stream_by_id(stream_id, user_profile.realm)
+    except JsonableError:
+        stream = None
     result = {"exists": bool(stream)}
     if stream is not None:
         recipient = get_recipient(Recipient.STREAM, stream.id)
@@ -487,6 +489,14 @@ def stream_exists_backend(request, user_profile, stream_name, autosubscribe):
         return json_success(result) # results are ignored for HEAD requests
     return json_response(data=result, status=404)
 
+def get_and_validate_stream_by_id(stream_id, realm):
+    # type: (int, Realm) -> Stream
+    try:
+        stream = Stream.objects.get(pk=stream_id, realm_id=realm.id)
+    except Stream.DoesNotExist:
+        raise JsonableError(_("Invalid stream id"))
+    return stream
+
 @has_request_variables
 def json_get_stream_id(request, user_profile, stream=REQ()):
     # type: (HttpRequest, UserProfile, Text) -> HttpResponse

zproject/urls.py

@@ -293,9 +293,9 @@ v1_api_and_json_patterns = [
         {'GET': 'zerver.views.streams.json_get_stream_id'}),
 
     # GET returns "stream info" (undefined currently?), HEAD returns whether stream exists (200 or 404)
-    url(r'^streams/(?P<stream_name>.*)/members$', rest_dispatch,
+    url(r'^streams/(?P<stream_id>\d+)/members$', rest_dispatch,
         {'GET': 'zerver.views.streams.get_subscribers_backend'}),
-    url(r'^streams/(?P<stream_name>.*)$', rest_dispatch,
+    url(r'^streams/(?P<stream_id>\d+)$', rest_dispatch,
         {'HEAD': 'zerver.views.streams.stream_exists_backend',
          'GET': 'zerver.views.streams.stream_exists_backend',
          'PATCH': 'zerver.views.streams.update_stream_backend',