fix(destinations): Handle unicode characters in webhook notifications

[jingyugao]

Previously, webhook notifications would fail if they contained unicode characters in the alert data. This was because the JSON payload was not UTF-8 encoded before being sent.

This commit fixes the issue by explicitly encoding the JSON data to UTF-8 and adds a test to verify the fix.

What type of PR is this?

• Refactor
• Feature
• Bug Fix
• New Query Runner (Data Source)
• New Alert Destination
• Other

Description

How is this tested?

• Unit tests (pytest, jest)
• E2E Tests (Cypress)
• Manually
• N/A

Related Tickets & Documents

#7468

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[yoshiokatsuneo]

@jingyugao

Thank you for your contribution!
I confirmed that the PR works well.

And, if we create a test, I think the test should be in the file related to the class, like test/destinations/test_webhook.py.

Also, I think the test is optional for this quite simple PR.

[jingyugao]

@jingyugao

Thank you for your contribution! I confirmed that the PR works well.

And, if we create a test, I think the test should be in the file related to the class, like test/destinations/test_webhook.py.

Also, I think the test is optional for this quite simple PR.

Thankyou for yout comment! tests/handlers/test_destinations.py is an already exists file which contain tests about webhook.Shall I create a new file in test/destinations/ ?

[yoshiokatsuneo]

@jingyugao
Thank you for your contribution! I confirmed that the PR works well.
And, if we create a test, I think the test should be in the file related to the class, like test/destinations/test_webhook.py.
Also, I think the test is optional for this quite simple PR.

Thankyou for yout comment! tests/handlers/test_destinations.py is an already exists file which contain tests about webhook.Shall I create a new file in test/destinations/ ?

I think so, if we add the test.

[arikfr]

Can you elaborate on "webhook notifications would fail if they contained unicode characters in the alert data" -- when would it fail?

I would imagine something like this was needed in Python 2, but in Python 3 it uses Unicode strings which supposed to "just" work.

It will be beneficial if the test was reproducing the actual issue. In this case the test verifies we send bytes to requests.post, but does not really show case the error.

[yoshiokatsuneo]

@arikfr

As written in the issue(#7468) PR author created, request.post failed when data parameter is UTF-8 string.
The error happens around the following code.

https://github.com/python/cpython/blob/main/Lib/http/client.py#L1405

        if isinstance(body, str):
            # RFC 2616 Section 3.7.1 says that text default has a
            # default charset of iso-8859-1.
            body = _encode(body, 'body')

https://github.com/python/cpython/blob/main/Lib/http/client.py#L166C1-L170C38

def _encode(data, name='data'):
    """Call data.encode("latin-1") but show a better error message."""
    try:
        return data.encode("latin-1")
    except UnicodeEncodeError as err:

And, following requests API reference says data should be "bytes"(not "string"). So, to follow the specification, we need to pass data as "bytes".

https://requests.readthedocs.io/en/latest/api/

data – (optional) Dictionary, list of tuples, bytes, or file-like object to send in the body of the [Request](https://requests.readthedocs.io/en/latest/api/#requests.Request)

Here is the stack trace when the error happens:

redash@9a75d904dc19:/app$ cat a2.py 
import requests

resp = requests.post(
    "https://example.com",
    data="你好",
    headers={},
    timeout=5.0,
)
print(resp.status_code)
redash@9a75d904dc19:/app$ python a2.py 
Traceback (most recent call last):
  File "/app/a2.py", line 3, in <module>
    resp = requests.post(
  File "/usr/local/lib/python3.10/site-packages/requests/api.py", line 115, in post
    return request("post", url, data=data, json=json, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/requests/api.py", line 59, in request
    return session.request(method=method, url=url, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/requests/sessions.py", line 589, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/local/lib/python3.10/site-packages/requests/sessions.py", line 703, in send
    r = adapter.send(request, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/requests/adapters.py", line 667, in send
    resp = conn.urlopen(
  File "/usr/local/lib/python3.10/site-packages/urllib3/connectionpool.py", line 715, in urlopen
    httplib_response = self._make_request(
  File "/usr/local/lib/python3.10/site-packages/urllib3/connectionpool.py", line 416, in _make_request
    conn.request(method, url, **httplib_request_kw)
  File "/usr/local/lib/python3.10/site-packages/urllib3/connection.py", line 244, in request
    super(HTTPConnection, self).request(method, url, body=body, headers=headers)
  File "/usr/local/lib/python3.10/http/client.py", line 1283, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/local/lib/python3.10/http/client.py", line 1328, in _send_request
    body = _encode(body, 'body')
  File "/usr/local/lib/python3.10/http/client.py", line 166, in _encode
    raise UnicodeEncodeError(
UnicodeEncodeError: 'latin-1' codec can't encode characters in position 0-1: Body ('你好') is not valid Latin-1. Use body.encode('utf-8') if you want to send it encoded in UTF-8.

[arikfr]

@yoshiokatsuneo thank you for the elaborate explanation!

For a moment I thought that maybe we should use the json field of post(), but realized that leveraging our own json_dumps has benefits, so I'm cool with the solution here.

[jingyugao]

@jingyugao
Thank you for your contribution! I confirmed that the PR works well.
And, if we create a test, I think the test should be in the file related to the class, like test/destinations/test_webhook.py.
Also, I think the test is optional for this quite simple PR.

Thankyou for yout comment! tests/handlers/test_destinations.py is an already exists file which contain tests about webhook.Shall I create a new file in test/destinations/ ?

I think so, if we add the test.

updated

[yoshiokatsuneo]

@jingyugao

I just approved and merged the PR.
Thank you for your contribution !