markciecior / ConnectPyse

ConnectWise (Manage) REST API client written in Python 3.x
MIT License
21 stars 8 forks source link

fix: always convert json payloads to ascii #30

Closed 0xliam closed 3 months ago

0xliam commented 4 months ago

The POST, PUT and PATCH methods do not convert all payloads to ASCII as ensure_ascii=False is set on the calls to json.dumps().

Currently, if you to send a POST or PATCH request that contains UTF-8 characters to the ConnectWise API, you receive an error.

e.g.

>>> data = [{"op": "replace", "path": "firstName", "value": "Liám"}]
>>> payload = json.dumps(
...   data, ensure_ascii=False # current connectpyse behaviour
... )
>>> response = requests.request("PATCH", url, headers=headers, data=payload)
>>> response.raise_for_status()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".venv/lib/python3.11/site-packages/requests/models.py", line 1021, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: https://redacted/v4_6_release/apis/3.0/company/contacts/5651
>>> response.json()
{'code': 'ConnectWiseApi', 'message': 'The JSON input is not in the expected array format for a JSON Patch document. A JSON Patch document should be a JSON array of patch operations. Please revise your JSON structure and try again.'}

If you set ensure_ascii=True, which is the default of json.dumps(), the request is accepted:

>>> payload = json.dumps(
...   data, ensure_ascii=True # default json behaviour
... )
>>>
>>> response = requests.request("PATCH", url, headers=headers, data=payload)
>>> response.raise_for_status()
>>> response.status_code
200

This PR replaces all usage of ensure_ascii=False with ensure_ascii=True.

I'm happy to flesh this out a bit more if you want this setting to be controlled by the user.

Note that I am testing against an on-prem instance of ConnectWise (v2024.6.100619).

markciecior commented 4 months ago

Hi @0xliam, thanks for the PR.

  1. For my own edification, what does the output look like in ConnectWise when you submit these UTF-8 characters? Does it display them correctly?
  2. I'm leery of making this change globally. What do you think about defining it as a kwarg instead so the user can optionally affect this when creating a controller? Something like this:
from connectpyse.sales import opportunity_api
o = opportunity_api.OpportunityAPI(url=URL, auth=AUTH, ensure_ascii=True)

Thoughts?

0xliam commented 4 months ago

Hi @markciecior, thanks for reviewing! ConnectWise does display these characters correctly when submitted as ASCII, however I agree it might be risky making the change globally - I've done a minimal amount of testing and it doesn't seem to have caused any problems, but I am happy make this a kwarg with the default as the existing behaviour.

I'll rework my PR and submit it later today! :)

0xliam commented 4 months ago

Hi @markciecior, I've reworked this PR as suggested - users can now instantiate the API with the ensure_ascii kwarg, or via the configuration file - and it will default to False, which is the existing behaviour.

0xliam commented 4 months ago

Hi @markciecior,

Just wondering if you've had a moment to review this PR? :)