From 9e06ab45bf065ff3b2abef45f871472c18fb497b Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Wed, 5 Dec 2018 15:12:19 -0800 Subject: [PATCH] webhooks: Fix HttpResponse with notify_bot_owner_on_invalid_json. Apparently, there was a bug in notify_bot_owner_on_invalid_json, where we didn't reraise the JsonableError. We fix this with a refactoring that makes the exception layering clearer as well. --- zerver/decorator.py | 17 ++++++++--------- zerver/tests/test_webhooks_common.py | 9 +++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/zerver/decorator.py b/zerver/decorator.py index 21a87514c6..4d3d046011 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -348,16 +348,15 @@ def api_key_only_webhook_view( rate_limit_user(request, user_profile, domain='all') try: return view_func(request, user_profile, *args, **kwargs) - except InvalidJSONError as e: - if not notify_bot_owner_on_invalid_json: - raise e - # NOTE: importing this at the top of file leads to a - # cyclic import; correct fix is probably to move - # notify_bot_owner_about_invalid_json to a smaller file. - from zerver.lib.webhooks.common import notify_bot_owner_about_invalid_json - notify_bot_owner_about_invalid_json(user_profile, webhook_client_name) except Exception as err: - log_exception_to_webhook_logger(request, user_profile) + if isinstance(err, InvalidJSONError) and notify_bot_owner_on_invalid_json: + # NOTE: importing this at the top of file leads to a + # cyclic import; correct fix is probably to move + # notify_bot_owner_about_invalid_json to a smaller file. + from zerver.lib.webhooks.common import notify_bot_owner_about_invalid_json + notify_bot_owner_about_invalid_json(user_profile, webhook_client_name) + else: + log_exception_to_webhook_logger(request, user_profile) raise err return _wrapped_func_arguments diff --git a/zerver/tests/test_webhooks_common.py b/zerver/tests/test_webhooks_common.py index 65d73ae440..43d1f77702 100644 --- a/zerver/tests/test_webhooks_common.py +++ b/zerver/tests/test_webhooks_common.py @@ -52,11 +52,11 @@ class WebhooksCommonTestCase(ZulipTestCase): def test_notify_bot_owner_on_invalid_json(self)-> None: @api_key_only_webhook_view('ClientName', notify_bot_owner_on_invalid_json=False) - def my_webhook_raises_exception(request: HttpRequest, user_profile: UserProfile) -> None: + def my_webhook_no_notify(request: HttpRequest, user_profile: UserProfile) -> None: raise InvalidJSONError("Malformed JSON") @api_key_only_webhook_view('ClientName', notify_bot_owner_on_invalid_json=True) - def my_webhook(request: HttpRequest, user_profile: UserProfile) -> None: + def my_webhook_notify(request: HttpRequest, user_profile: UserProfile) -> None: raise InvalidJSONError("Malformed JSON") webhook_bot_email = 'webhook-bot@zulip.com' @@ -70,7 +70,7 @@ class WebhooksCommonTestCase(ZulipTestCase): last_message_id = self.get_last_message().id with self.assertRaisesRegex(JsonableError, "Malformed JSON"): - my_webhook_raises_exception(request) # type: ignore # mypy doesn't seem to apply the decorator + my_webhook_no_notify(request) # type: ignore # mypy doesn't seem to apply the decorator # First verify that without the setting, it doesn't send a PM to bot owner. msg = self.get_last_message() @@ -78,7 +78,8 @@ class WebhooksCommonTestCase(ZulipTestCase): self.assertNotEqual(msg.content, expected_msg.strip()) # Then verify that with the setting, it does send such a message. - my_webhook(request) # type: ignore # mypy doesn't seem to apply the decorator + with self.assertRaisesRegex(JsonableError, "Malformed JSON"): + my_webhook_notify(request) # type: ignore # mypy doesn't seem to apply the decorator msg = self.get_last_message() self.assertNotEqual(msg.id, last_message_id) self.assertEqual(msg.sender.email, self.notification_bot().email)