From 4c5eb3d06e62703b829fcfbd7afd24cc0f2dd3f1 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Mon, 12 Sep 2016 08:21:49 -0700 Subject: [PATCH] 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 | 1 + zerver/tests/test_subs.py | 42 +++++++++++++++++++++++++++++++++++++++ zerver/views/streams.py | 36 +++++++++++++++++++++++---------- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/tools/lint-all b/tools/lint-all index e1ab512c85..19b8acebfa 100755 --- a/tools/lint-all +++ b/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[\'"]', diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 193fc49233..60ed17ffd9 100644 --- a/zerver/tests/test_subs.py +++ b/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): diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 23a47be933..f6edccf50b 100644 --- a/zerver/views/streams.py +++ b/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