Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patch occasional UnicodeEncodeError that occurs with outgoing webhooks #3832

Merged
merged 4 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

### Fixed

- Address outgoing webhook encoding error when passing non-latin characters in the webhook request body by @joeyorlando
([#TBD](https://github.com/grafana/oncall/pull/TBD))

## v1.3.100 (2024-02-01)

### Added
Expand Down
3 changes: 2 additions & 1 deletion engine/apps/webhooks/models/webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,8 @@ def build_request_kwargs(self, event_data, raise_data_errors=False):
try:
request_kwargs["json"] = json.loads(rendered_data)
except (JSONDecodeError, TypeError):
request_kwargs["data"] = rendered_data
# utf-8 encoding addresses https://github.com/grafana/oncall/issues/3831
request_kwargs["data"] = rendered_data.encode("utf-8")
except (JinjaTemplateError, JinjaTemplateWarning) as e:
if raise_data_errors:
raise InvalidWebhookData(e.fallback_message)
Expand Down
72 changes: 55 additions & 17 deletions engine/apps/webhooks/tests/test_trigger_webhook.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import json
from unittest.mock import call, patch

import httpretty
import pytest
import requests
from django.utils import timezone
Expand Down Expand Up @@ -130,51 +131,88 @@ def test_execute_webhook_integration_filter_matching(
)


ALERT_GROUP_PUBLIC_PRIMARY_KEY = "IXJ47FKMYYJ5U"


@httpretty.activate(verbose=True, allow_net_connect=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the exception I was trying to fix, and fully test, here happens rather deep inside the requests package (logs). Rather than trying to figure out what to properly mock there I decided to try out httpretty which according to their docs:

It can be a bit of a hassle to use something like mock.Mock to stub the requests, this can work well for low-level unit tests but when writing functional or integration tests we should be able to allow the http calls to go through the TCP socket module.

HTTPretty monkey patches Python’s socket core module with a fake version of the module.

Because HTTPretty implements a fake the modules socket and ssl you can use write tests to code against any HTTP library that use those modules.

in other words, we don't have to patch requests at all (note: I'm patching requests but w/ the wraps kwarg which basically sets a "spy" on the package and doesn't mock its actual implementation (docs))

@pytest.mark.parametrize(
"data,expected_request_data,request_post_kwargs",
[
(
'{"value": "{{ alert_group_id }}"}',
json.dumps({"value": ALERT_GROUP_PUBLIC_PRIMARY_KEY}),
{"json": {"value": ALERT_GROUP_PUBLIC_PRIMARY_KEY}},
),
# test that non-latin characters are properly encoded
(
"😊",
"b'\\xf0\\x9f\\x98\\x8a'",
{"data": "😊".encode("utf-8")},
),
],
)
@pytest.mark.django_db
def test_execute_webhook_ok(
make_organization, make_user_for_organization, make_alert_receive_channel, make_alert_group, make_custom_webhook
make_organization,
make_user_for_organization,
make_alert_receive_channel,
make_alert_group,
make_custom_webhook,
data,
expected_request_data,
request_post_kwargs,
):
organization = make_organization()
user = make_user_for_organization(organization)
alert_receive_channel = make_alert_receive_channel(organization)
alert_group = make_alert_group(
alert_receive_channel, acknowledged_at=timezone.now(), acknowledged=True, acknowledged_by=user.pk
alert_receive_channel,
acknowledged_at=timezone.now(),
acknowledged=True,
acknowledged_by=user.pk,
public_primary_key=ALERT_GROUP_PUBLIC_PRIMARY_KEY,
)
webhook = make_custom_webhook(
organization=organization,
url="https://something/{{ alert_group_id }}/",
url="https://example.com/{{ alert_group_id }}/",
http_method="POST",
trigger_type=Webhook.TRIGGER_ACKNOWLEDGE,
trigger_template="{{{{ alert_group.integration_id == '{}' }}}}".format(
alert_receive_channel.public_primary_key
),
headers='{"some-header": "{{ alert_group_id }}"}',
data='{"value": "{{ alert_group_id }}"}',
data=data,
forward_all=False,
)

mock_response = MockResponse()
with patch("apps.webhooks.utils.socket.gethostbyname") as mock_gethostbyname:
mock_gethostbyname.return_value = "8.8.8.8"
with patch("apps.webhooks.models.webhook.requests") as mock_requests:
mock_requests.post.return_value = mock_response
templated_url = f"https://example.com/{alert_group.public_primary_key}/"
mock_response = httpretty.Response(json.dumps({"response": 200}))
httpretty.register_uri(httpretty.POST, templated_url, responses=[mock_response])

with patch("apps.webhooks.utils.socket.gethostbyname", return_value="8.8.8.8"):
with patch("apps.webhooks.models.webhook.requests", wraps=requests) as mock_requests:
execute_webhook(webhook.pk, alert_group.pk, user.pk, None)

assert mock_requests.post.called
expected_call = call(
"https://something/{}/".format(alert_group.public_primary_key),
mock_requests.post.assert_called_once_with(
templated_url,
timeout=TIMEOUT,
headers={"some-header": alert_group.public_primary_key},
json={"value": alert_group.public_primary_key},
**request_post_kwargs,
)
assert mock_requests.post.call_args == expected_call

# assert the request was made to the webhook as we expected
last_request = httpretty.last_request()
assert last_request.method == "POST"
assert last_request.url == templated_url
assert last_request.headers["some-header"] == alert_group.public_primary_key

# check logs
log = webhook.responses.all()[0]
assert log.status_code == 200
assert log.content == json.dumps(mock_response.json())
assert log.request_data == json.dumps({"value": alert_group.public_primary_key})
assert log.content == json.dumps({"response": 200})
assert log.request_data == expected_request_data
assert log.request_headers == json.dumps({"some-header": alert_group.public_primary_key})
assert log.url == "https://something/{}/".format(alert_group.public_primary_key)
assert log.url == templated_url
# check log record
log_record = alert_group.log_records.last()
assert log_record.type == AlertGroupLogRecord.TYPE_CUSTOM_BUTTON_TRIGGERED
Expand Down
1 change: 1 addition & 0 deletions engine/requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ types-beautifulsoup4==4.12.0.5
types-PyMySQL==1.0.19.7
types-python-dateutil==2.8.19.13
types-requests==2.31.0.1
httpretty==1.1.4
Loading