Skip to content
New issue

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

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

Already on GitHub? # to your account

🐛 Specify utf-8 webhook encoding #299

Closed
wants to merge 1 commit into from
Closed

Conversation

zdwolfe
Copy link

@zdwolfe zdwolfe commented Sep 15, 2023

Description

Fixes #302

Specify utf-8 body encoding on webhook requests. The emojis in campsites json body cannot be encoded with latin-1 encoding and camply crashes. The utf-8 encoding is the json spec character encoding, so it seems like an OK default stance for camply to take.

JSON text exchanged between systems that are not part of a closed ecosystem MUST be encoded using UTF-8

Before

 WEBHOOK_URL="http://localhost:8000" camply campsites --campground 233998 --start-date 2023-10-10 --end-date 2023-10-21  --notifications webhook

..would crash with

RuntimeError: camply encountered an error and exited 😟 [2023-09-14 22:13:53] - (UnicodeEncodeError) 'latin-1' codec
can't encode character '\U0001f3d5' in position 161: Body ('🏕') is not valid Latin-1. Use body.encode('utf-8') if you
want to send it encoded in UTF-8.

Has This Been Tested?

In background shell, start a simple webhook handler

from http.server import BaseHTTPRequestHandler, HTTPServer
import socketserver

class PostHandler(BaseHTTPRequestHandler):
    def do_POST(self):
        print('request')
        content_length = int(self.headers['Content-Length'])
        post_data = self.rfile.read(content_length)
        print(post_data.decode('utf-8'))

        self.send_response(200)
        self.end_headers()
        self.wfile.write(b'OK')

if __name__ == "__main__":
    PORT = 8000
    with HTTPServer(("", PORT), PostHandler) as httpd:
        print("serving at port", PORT)
        httpd.serve_forever()
(camply) (base) zach@z-pc:~/src/camply$ git log --oneline | head -n 1
f3b51ae 🐛 Specify utf-8 webhook encoding

then run:

(camply) (base) zach@z-pc:~/src/camply$ hatch shell
(camply) (base) zach@z-pc:~/src/camply$ WEBHOOK_URL="http://localhost:8000" camply campsites --campground 233998 --start-date 2023-10-10 --end-date 2023-10-21  --notifications webhook

..and the webhook handler prints:

127.0.0.1 - - [14/Sep/2023 22:17:15] "POST / HTTP/1.1" 200 -
request
{"campsites": [{"campsite_id": 76540, "booking_date": "2023-10-12T00:00:00",
......

..also,

(camply) (base) zach@z-pc:~/src/camply$ hatch run all
...
-------------------------------------------------------------------------------------------------------
TOTAL                                                        3895    627    956    139    81%

...
108 passed in 30.67s 
...

Checklist:

  • I've read the contributing guidelines of this project
  • I've installed and used .pre_commit on all my code
  • I have documented my code, particularly in hard-to-understand areas
  • I have made any necessary corresponding changes to the documentation (none needed, I think utf8 for json is well understood)

(also, this is the first GitHub PR I've ever made, so apologies if something's not quite right).

@juftin
Copy link
Owner

juftin commented Sep 23, 2023

Nice, so this works @zdwolfe but I was thinking of a different approach.

The actual message that broke the utf-8 encoding wasn't a standard webhook body, it was a text message sent by send_message. Something like this:

Found more than 5 matching campsites (16) on the first try. Try searching online instead. camply is only sending the first 5 notifications. Go Get your campsite! 🏕
image

I'm wondering if we should be disabling send_message altogether with a no-op and only enabling send_campsites. This way the ony messages that get sent to the webhook are valid JSON campsites. We can also ensure utf-8 encoding while we're at it too.

Let me know what you think?

@zdwolfe
Copy link
Author

zdwolfe commented Sep 23, 2023

Oh, I see! Disabling send_message sounds good to me, though I also agree utf-8 might be worth keeping around.

@juftin
Copy link
Owner

juftin commented Sep 23, 2023

Resolved by #303

@juftin juftin closed this Sep 23, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webhook crashes on utf-8 characters
2 participants