Fix transaction behavior for update_subscriptions_backend().

This commit extracts compose_views() from update_subscriptions_backend(),
and it implements the correct behavior for forcing transactions to roll
back, which is to raise an exception.

There were really three steps in this commit:

- Extract buggy code to compose_views().
- Add tests on compose_views().
- Fix bugs exposed by the new tests by converting errors to exceptions.

tools/lint-all

@@ -288,6 +288,7 @@ def build_custom_checkers(by_lang):
              ('zerver/lib/actions.py', 'raise JsonableError(e.messages[0])'),
              ('zerver/views/messages.py', 'raise JsonableError(error)'),
              ('zerver/lib/request.py', 'raise JsonableError(error)'),
+             ('zerver/views/streams.py', 'raise JsonableError(response.content)'),
          ]),
          'description': 'Argument to JsonableError should be a literal string enclosed by _()'},
         {'pattern': '([a-zA-Z0-9_]+)=REQ\([\'"]\\1[\'"]',

zerver/tests/test_subs.py

@@ -3,6 +3,9 @@ from __future__ import absolute_import
 
 from typing import Any, Dict, List, Mapping, Optional, Sequence
 
+from django.http import HttpRequest, HttpResponse
+from django.utils.translation import ugettext as _
+
 from zerver.lib import cache
 
 from zerver.lib.test_helpers import (
@@ -13,6 +16,11 @@ from zerver.decorator import (
     JsonableError
 )
 
+from zerver.lib.response import (
+    json_error,
+    json_success,
+)
+
 from zerver.lib.test_runner import (
     slow
 )
@@ -30,6 +38,10 @@ from zerver.lib.actions import (
     get_user_profile_by_email, set_default_streams, get_subscription
 )
 
+from zerver.views.streams import (
+    compose_views
+)
+
 from django.http import HttpResponse
 import mock
 import random
@@ -881,6 +893,36 @@ class SubscriptionRestApiTest(ZulipTestCase):
         self.assert_json_error(result,
                                "Stream name (%s) too long." % (long_stream_name,))
 
+    def test_compose_views_rollback(self):
+        # type: () -> None
+        '''
+        The compose_views function() is used under the hood by
+        update_subscriptions_backend.  It's a pretty simple method in terms of
+        control flow, but it uses a Django rollback, which may make it brittle
+        code when we upgrade Django.  We test the functions's rollback logic
+        here with a simple scenario to avoid false positives related to
+        subscription complications.
+        '''
+        user_profile = get_user_profile_by_email('hamlet@zulip.com')
+        user_profile.full_name = 'Hamlet'
+        user_profile.save()
+
+        def method1 (req, user_profile):
+            # type: (HttpRequest, UserProfile) -> HttpResponse
+            user_profile.full_name = 'Should not be committed'
+            user_profile.save()
+            return json_success({})
+
+        def method2(req, user_profile):
+            # type: (HttpRequest, UserProfile) -> HttpResponse
+            return json_error(_('random failure'))
+
+        with self.assertRaises(JsonableError):
+            compose_views(None, user_profile, [(method1, {}), (method2, {})])
+
+        user_profile = get_user_profile_by_email('hamlet@zulip.com')
+        self.assertEqual(user_profile.full_name, 'Hamlet')
+
 class SubscriptionAPITest(ZulipTestCase):
 
     def setUp(self):

zerver/views/streams.py

@@ -1,5 +1,5 @@
 from __future__ import absolute_import
-from typing import Any, Optional, Tuple, List, Set, Iterable, Mapping, Callable
+from typing import Any, Optional, Tuple, List, Set, Iterable, Mapping, Callable, Dict
 
 from django.utils.translation import ugettext as _
 from django.conf import settings
@@ -181,9 +181,8 @@ def list_subscriptions_backend(request, user_profile):
     # type: (HttpRequest, UserProfile) -> HttpResponse
     return json_success({"subscriptions": gather_subscriptions(user_profile)[0]})
 
-FuncItPair = Tuple[Callable[..., HttpResponse], Iterable[Any]]
+FuncKwargPair = Tuple[Callable[..., HttpResponse], Dict[str, Iterable[Any]]]
 
-@transaction.atomic
 @has_request_variables
 def update_subscriptions_backend(request, user_profile,
                                  delete=REQ(validator=check_list(check_string), default=[]),
@@ -192,14 +191,31 @@ def update_subscriptions_backend(request, user_profile,
     if not add and not delete:
         return json_error(_('Nothing to do. Specify at least one of "add" or "delete".'))
 
+    method_kwarg_pairs = [
+        (add_subscriptions_backend, dict(streams_raw=add)),
+        (remove_subscriptions_backend, dict(streams_raw=delete))
+    ] # type: List[FuncKwargPair]
+    return compose_views(request, user_profile, method_kwarg_pairs)
+
+def compose_views(request, user_profile, method_kwarg_pairs):
+    # type: (HttpRequest, UserProfile, List[FuncKwargPair]) -> HttpResponse
+    '''
+    This takes a series of view methods from method_kwarg_pairs and calls
+    them in sequence, and it smushes all the json results into a single
+    response when everything goes right.  (This helps clients avoid extra
+    latency hops.)  It rolls back the transaction when things go wrong in
+    any one of the composed methods.
+
+    TODO: Move this a utils-like module if we end up using it more widely.
+    '''
+
     json_dict = {} # type: Dict[str, Any]
-    method_items_pairs = ((add_subscriptions_backend, add), (remove_subscriptions_backend, delete)) # type: Tuple[FuncItPair, FuncItPair]
-    for method, items in method_items_pairs:
-        response = method(request, user_profile, streams_raw=items)
-        if response.status_code != 200:
-            transaction.rollback()
-            return response
-        json_dict.update(ujson.loads(response.content))
+    with transaction.atomic():
+        for method, kwargs in method_kwarg_pairs:
+            response = method(request, user_profile, **kwargs)
+            if response.status_code != 200:
+                raise JsonableError(response.content)
+            json_dict.update(ujson.loads(response.content))
     return json_success(json_dict)
 
 @authenticated_json_post_view