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

fix: discord webhook embed timestamp fails to serialize #555

Closed
wants to merge 4 commits into from

Conversation

minisbett
Copy link
Contributor

Describe your changes

The timestamp of embeds on a webhook fail to serialize into JSON properly.

Checklist

  • I've manually tested my code

@tsunyoku
Copy link
Contributor

tsunyoku commented Jan 30, 2024

these are like syntactically identical under the hood so i'd like to understand what the issue is

i'm not convinced there's an issue when this exists:

image

@cmyui
Copy link
Member

cmyui commented Jan 30, 2024

have a sample error traceback for the issue @minisbett?

@cmyui
Copy link
Member

cmyui commented Jan 30, 2024

have a sample error traceback for the issue @minisbett?

Ah I found in discord,

bancho_1  | Traceback (most recent call last):
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/tenacity/_asyncio.py", line 50, in __call__
bancho_1  |     result = await fn(*args, **kwargs)
bancho_1  |              ^^^^^^^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/srv/root/app/discord.py", line 188, in post
bancho_1  |     response = await services.http_client.post(
bancho_1  |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/httpx/_client.py", line 1848, in post
bancho_1  |     return await self.request(
bancho_1  |            ^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/httpx/_client.py", line 1517, in request
bancho_1  |     request = self.build_request(
bancho_1  |               ^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/httpx/_client.py", line 358, in build_request
bancho_1  |     return Request(
bancho_1  |            ^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/httpx/_models.py", line 339, in __init__
bancho_1  |     headers, stream = encode_request(
bancho_1  |                       ^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/httpx/_content.py", line 214, in encode_request
bancho_1  |     return encode_json(json)
bancho_1  |            ^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/httpx/_content.py", line 177, in encode_json
bancho_1  |     body = json_dumps(json).encode("utf-8")
bancho_1  |            ^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/json/__init__.py", line 231, in dumps
bancho_1  |     return _default_encoder.encode(obj)
bancho_1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/json/encoder.py", line 200, in encode
bancho_1  |     chunks = self.iterencode(o, _one_shot=True)
bancho_1  |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/json/encoder.py", line 258, in iterencode
bancho_1  |     return _iterencode(o, 0)
bancho_1  |            ^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/json/encoder.py", line 180, in default
bancho_1  |     raise TypeError(f'Object of type {o.__class__.__name__} '
bancho_1  | TypeError: Object of type datetime is not JSON serializable
bancho_1  | 
bancho_1  | The above exception was the direct cause of the following exception:
bancho_1  | 
bancho_1  | Traceback (most recent call last):
bancho_1  |   File "/srv/root/app/commands.py", line 2499, in process_commands
bancho_1  |     res = await cmd.callback(
bancho_1  |           ^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/srv/root/app/commands.py", line 691, in _map
bancho_1  |     await webhook.post()
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/tenacity/_asyncio.py", line 88, in async_wrapped
bancho_1  |     return await fn(*args, **kwargs)
bancho_1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/tenacity/_asyncio.py", line 47, in __call__
bancho_1  |     do = self.iter(retry_state=retry_state)
bancho_1  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
bancho_1  |   File "/usr/local/lib/python3.11/site-packages/tenacity/__init__.py", line 326, in iter
bancho_1  |     raise retry_exc from fut.exception()
bancho_1  | tenacity.RetryError: RetryError[<Future at 0x7f1ba1c8c4d0 state=finished raised TypeError>]

@cmyui
Copy link
Member

cmyui commented Jan 30, 2024

Did you migrate your bancho.py to use httpx? Looks like master is using aiohttp and the exception mentions httpx.

@cmyui
Copy link
Member

cmyui commented Jan 30, 2024

Ah nvm I understand - @tsunyoku I think you linked non-master code

@tsunyoku
Copy link
Contributor

tsunyoku commented Jan 30, 2024

ok can we fix by changing httpx's serialization then?

Copy link
Member

@cmyui cmyui left a comment

Choose a reason for hiding this comment

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

As @tsunyoku touched on ^, having this as a global configuration for the JSON serialization of the services.http_client would be a more desirable change - as this issue may exist in several other code spots.

I suspect there should be a way to hook the JSON serialization - we shouldn't need to use httpx.request(content=...).

@minisbett
Copy link
Contributor Author

as this issue may exist in several other code spots

yup, getting that error elsewhere as well.

@cmyui
Copy link
Member

cmyui commented Jan 31, 2024

I found this comment/discussion while browsing the issues on the httpx repo:

I think this comment is true - it should be possible to subclass httpx.AsyncClient to customize the serialization

@cmyui cmyui marked this pull request as draft February 4, 2024 08:59
@cmyui
Copy link
Member

cmyui commented Feb 13, 2024

Hm, in the current httpx lib there isn't really a nice way to do this since request_class and response_class aren't configurable.

We can subclass AsyncClient to be able to hook BaseClient.build_request to use a custom Request class, but it involves quite a bit of copying from httpx.

Alternatively we could hook httpx._content.encode_json, but it's not ideal -- clearly not a public api and so it may change in a non-breaking release.

Might actually be best overall to make a PR to httpx to add support for request_class and response_class, then as tomchristie proposes, we can do something like:

class APIClient(httpx.Client):
    request_class = APIRequest
    response_class = APIResponse

class APIRequest(httpx.Request):
    def __init__(self, *args, **kwargs):
        if 'json' in kwargs:
            content = orjson.dumps(kwargs.pop('json'))
            headers = kwargs.get('headers', {})
            headers['Content-Length'] = len(content)
            kwargs['content'] = content
            kwargs['headers'] = headers
        return super().__init__(*args, **kwargs)

class APIResponse(httpx.Response):
    def json(self):
        return orjson.loads(self.content)

@cmyui cmyui self-assigned this Feb 13, 2024
@cmyui
Copy link
Member

cmyui commented Feb 13, 2024

I'll turn this into an issue as I think implementation is a bit pre-mature - will tag everyone there for further discussion

@cmyui cmyui closed this Feb 13, 2024
@minisbett minisbett deleted the patch-1 branch February 26, 2024 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants