ConvertAPI / convertapi-library-python

A Python library for the ConvertAPI
https://www.convertapi.com
Other
73 stars 22 forks source link

JSONDecodeError when getting/polling a 202/Conversion in progress response #21

Open michelts opened 3 years ago

michelts commented 3 years ago

I'm using the asynchronous conversion mode and I'm using the convertpi.client.get method to poll the conversion result.

I'm polling the results only after receiving the webhook confirmation, understanding that the conversion process would have been successful in this case.

For some reason, when polling results from the API after the webhook confirmation, I'm getting a response different than 200 OK. These are the possible status codes:

I mostly get 200 OK responses when polling after the confirmation, but there are cases where I get a 202 Accepted with blank content. I suppose I'm hitting the API too fast and there is some internal propagation still ongoing, but that is not the point anyway, I could simply retry it and it will work.

The problem is that, since the response content is a blank string, and since the get method uses the handle_response method to convert the response to JSON or raise an exception, I'm hitting the unexpected exception json.decoder.JSONDecodeError.

The other error responses would _raise_forstatus and fail legitimely. The only problem is with the 202 response, that is a successful status (so it is not raised), but can't be returned as json. I wrote a small wrapper to solve this at my side:

def poll_retrieve(job_id):
    convertapi.api_secret = settings.CONVERT_API_SECRET
    response = requests.get(
        convertapi.client.url(f"job/{job_id}"),
        headers=convertapi.client.headers(),
        timeout=convertapi.timeout or None,
    )
    response.raise_for_status()
    if response.status_code == 202:
        raise ConversionInProgressError
    return response.json()

I could write a pull request, but I want to discuss with you the best approach first: is a new exception (like this ConversionInProgressError) a good idea? Or do you think returning a blank list would work better?

michelts commented 3 years ago

Notice that I'm ignoring the reason you are returning a 202. It might be a problem too, but anyway, I believe the API should be able to deal with this 202 Accepted response, regardless of how it was created.

tomasr78 commented 3 years ago

The 202 status code means that Async conversion is in progress and from what I read the problem is that 202 does not has a body and your JSON decoder threats 202 as a success and tries to decode JSON and gets an exception on the empty body.

We could add JSON response 202 and that should fix the problem. Let me know if I understood the problem and if my suggested fix will work for you.

michelts commented 3 years ago

Hi @tomasr78

tries to decode JSON and gets an exception

Exactly!

We could add JSON response 202 and that should fix the problem

I'm not sure I follow you. I was doing:

job_id = "the job id received through the webhook call"
resp = convertapi.client.get(f"job/{job_id}")

As you noticed, a 202 response with blank content would fail. A 200 response would return something like this:

{"Files": [...]}

I see some alternatives:

  1. Raise an explicit exception (e.g. ConversionInProgressError)
  2. Return blank Files list, so one would assume the conversion is in progress: {"Files": []}
  3. Create a raw_get that returns the full requests object: I would still get some benefits of the SDK (e.g. parametrization of the request), but would handle the response myself.

I'm personally in favor of an exception ;)