mirror of https://github.com/zulip/zulip.git
outgoing_webhook: Improve invalid json handling when parsing response.
It's better to just raise JsonableError here, as that makes this error processed in the central place for this kind of thing in do_rest_call: --------- except JsonableError as e: response_message = e.msg logging.info("Outhook trigger failed:", stack_info=True) fail_with_message(event, response_message) response_message = f"The outgoing webhook server attempted to send a message in Zulip, but that request resulted in the following error:\n> {e}" notify_bot_owner(event, failure_message=response_message) return None ---------- which does all the things that are supposed to happen - fail_with_message, appropriate logging and notifying the bot owner.
This commit is contained in:
parent
f1a3f6056a
commit
b7b1ec0aeb
|
@ -288,8 +288,7 @@ def process_success_response(
|
||||||
try:
|
try:
|
||||||
response_json = json.loads(response.text)
|
response_json = json.loads(response.text)
|
||||||
except json.JSONDecodeError:
|
except json.JSONDecodeError:
|
||||||
fail_with_message(event, "Invalid JSON in response")
|
raise JsonableError(_("Invalid JSON in response"))
|
||||||
return
|
|
||||||
|
|
||||||
if not isinstance(response_json, dict):
|
if not isinstance(response_json, dict):
|
||||||
raise JsonableError(_("Invalid response format"))
|
raise JsonableError(_("Invalid response format"))
|
||||||
|
|
|
@ -5,6 +5,7 @@ from unittest import mock
|
||||||
import requests
|
import requests
|
||||||
|
|
||||||
from zerver.lib.avatar import get_gravatar_url
|
from zerver.lib.avatar import get_gravatar_url
|
||||||
|
from zerver.lib.exceptions import JsonableError
|
||||||
from zerver.lib.message import MessageDict
|
from zerver.lib.message import MessageDict
|
||||||
from zerver.lib.outgoing_webhook import get_service_interface_class, process_success_response
|
from zerver.lib.outgoing_webhook import get_service_interface_class, process_success_response
|
||||||
from zerver.lib.test_classes import ZulipTestCase
|
from zerver.lib.test_classes import ZulipTestCase
|
||||||
|
@ -47,13 +48,12 @@ class TestGenericOutgoingWebhookService(ZulipTestCase):
|
||||||
response.status_code = 200
|
response.status_code = 200
|
||||||
response.text = "unparsable text"
|
response.text = "unparsable text"
|
||||||
|
|
||||||
with mock.patch("zerver.lib.outgoing_webhook.fail_with_message") as m:
|
with self.assertRaisesRegex(JsonableError, "Invalid JSON in response"):
|
||||||
process_success_response(
|
process_success_response(
|
||||||
event=event,
|
event=event,
|
||||||
service_handler=service_handler,
|
service_handler=service_handler,
|
||||||
response=response,
|
response=response,
|
||||||
)
|
)
|
||||||
self.assertTrue(m.called)
|
|
||||||
|
|
||||||
def test_make_request(self) -> None:
|
def test_make_request(self) -> None:
|
||||||
othello = self.example_user("othello")
|
othello = self.example_user("othello")
|
||||||
|
|
|
@ -288,6 +288,35 @@ The outgoing webhook server attempted to send a message in Zulip, but that reque
|
||||||
assert bot_user.bot_owner is not None
|
assert bot_user.bot_owner is not None
|
||||||
self.assertEqual(bot_owner_notification.recipient_id, bot_user.bot_owner.recipient_id)
|
self.assertEqual(bot_owner_notification.recipient_id, bot_user.bot_owner.recipient_id)
|
||||||
|
|
||||||
|
def test_invalid_json_in_response(self) -> None:
|
||||||
|
bot_user = self.example_user("outgoing_webhook_bot")
|
||||||
|
mock_event = self.mock_event(bot_user)
|
||||||
|
service_handler = GenericOutgoingWebhookService("token", bot_user, "service")
|
||||||
|
|
||||||
|
expect_logging_info = self.assertLogs(level="INFO")
|
||||||
|
expect_fail = mock.patch("zerver.lib.outgoing_webhook.fail_with_message")
|
||||||
|
|
||||||
|
with responses.RequestsMock(assert_all_requests_are_fired=True) as requests_mock:
|
||||||
|
# We mock the endpoint to return response with a body which isn't valid json.
|
||||||
|
requests_mock.add(
|
||||||
|
requests_mock.POST,
|
||||||
|
"https://example.zulip.com",
|
||||||
|
status=200,
|
||||||
|
body="this isn't valid json",
|
||||||
|
)
|
||||||
|
with expect_logging_info, expect_fail as mock_fail:
|
||||||
|
do_rest_call("https://example.zulip.com", mock_event, service_handler)
|
||||||
|
self.assertTrue(mock_fail.called)
|
||||||
|
bot_owner_notification = self.get_last_message()
|
||||||
|
self.assertEqual(
|
||||||
|
bot_owner_notification.content,
|
||||||
|
"""[A message](http://zulip.testserver/#narrow/stream/999-Verona/topic/Foo/near/) to your bot @_**Outgoing Webhook** triggered an outgoing webhook.
|
||||||
|
The outgoing webhook server attempted to send a message in Zulip, but that request resulted in the following error:
|
||||||
|
> Invalid JSON in response""",
|
||||||
|
)
|
||||||
|
assert bot_user.bot_owner is not None
|
||||||
|
self.assertEqual(bot_owner_notification.recipient_id, bot_user.bot_owner.recipient_id)
|
||||||
|
|
||||||
|
|
||||||
class TestOutgoingWebhookMessaging(ZulipTestCase):
|
class TestOutgoingWebhookMessaging(ZulipTestCase):
|
||||||
def create_outgoing_bot(self, bot_owner: UserProfile) -> UserProfile:
|
def create_outgoing_bot(self, bot_owner: UserProfile) -> UserProfile:
|
||||||
|
|
Loading…
Reference in New Issue