Handle getDetails JSON error on 5xx status code

[PriOliveira]

Should solve #107

[abdullahdevrel]

Thank you very much for the PR, Priscila. We really appreciate your help! I did not see the original issue; my apologies for the late response.

@max-ipinfo, can you kindly review the original issue (#107) and this PR, please? Everything looks good to me. I have documented our API error codes here, if you want to take a look: https://ipinfo.io/developers/openapi.yaml Thank you.

[max-ipinfo]

Thank you @PriOliveira for filing #107 and for sending a PR for this change! Your solution definitely improves the robustness of our library.

I would like to generalize and improve your solution further by explicitly handling non-JSON responses. Could you verify that it works on your end?

[max-ipinfo]

The only issue with this approach is that we lose the error details when response.status_code >= 500.

A better approach is to explicitly handle the response content type and extract the error message.

Could you change the lines (and the same in handler_async.py) to:

if response.status_code == 429:
    raise RequestQuotaExceededError()
if response.status_code >= 400:
    error_code = response.status_code
    content_type = response.headers.get('Content-Type')
    if content_type == 'application/json':
        error_response = response.json()
    else:
        error_response = {'error': response.text}
    raise APIError(error_code, json.dumps(error_response))

Thanks!

[PriOliveira]

Sure, good point. I'll update.

[max-ipinfo]

Two final formatting/linting nitpicks:

• remove blank lines 127 and 134 (and lines 149 and 156 in handler_async.py)
• even though HTTP headers are case-insensitive, we prefer to use the capitalized form in our code:

content_type = response.headers.get('Content-Type')

Thanks!

[PriOliveira]

For the header, I was trying to follow the existing pattern on those files. I only saw examples with lower case there.

[max-ipinfo]

Sorry for the confusion, @PriOliveira 🤦 ! I'll update the older lowercase convention in our code in a follow-up PR.

[PriOliveira]

I added some tests to try to cover those cases, hope you don't mind.

[max-ipinfo]

First off, sorry for my lack of context @PriOliveira.

I am just starting with this repository and do not have a good understanding of the current conventions. Bear with me 😄

I added some tests to try to cover those cases, hope you don't mind.

I ran pytest and getting these errors (with version 3.12.4):

$ pytest
============================================================================================= short test summary info =============================================================================================
FAILED tests/handler_async_test.py::test_get_details_error[5xx_not_json] - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'
FAILED tests/handler_async_test.py::test_get_details_error[4xx_json] - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'
FAILED tests/handler_async_test.py::test_get_details_error[400] - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'
FAILED tests/handler_async_test.py::test_get_details_quota_error - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'
FAILED tests/handler_async_test.py::test_get_batch_details[None] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details[1] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details[2] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details[3] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[None] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[1] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[2] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[3] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
==================================================================================== 12 failed, 36 passed, 4 warnings in 5.49s ====================================================================================
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x110cfde50>
sys:1: RuntimeWarning: coroutine 'AsyncHandler._do_batch_req' was never awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x110cb5fa0>
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x110cfff50>

Do these tests pass on your end? Could you share the module versions you are using?

I will most likely migrate to Poetry and upgrade dependency versions in the future.

Thanks again!

[max-ipinfo]

I ran pytest at HEAD and getting the following errors:

============================================================================================= short test summary info =============================================================================================
FAILED tests/handler_async_test.py::test_get_batch_details[None] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details[1] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details[2] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details[3] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[None] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[1] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[2] - TypeError: Passing coroutines is forbidden, use tasks explicitly.
FAILED tests/handler_async_test.py::test_get_batch_details_total_timeout[3] - TypeError: Passing coroutines is forbidden, use tasks explicitly.

Sorry!

So feel free to only fix the following errors:

FAILED tests/handler_async_test.py::test_get_details_error[5xx_not_json] - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'
FAILED tests/handler_async_test.py::test_get_details_error[4xx_json] - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'
FAILED tests/handler_async_test.py::test_get_details_error[400] - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'
FAILED tests/handler_async_test.py::test_get_details_quota_error - AttributeError: 'MockResponse' object has no attribute 'wait_for_close'

[max-ipinfo]

I dug further and noticed that our CI pipeline and requirements.txt mention using v3.10.

After switching to v3.10 and running pip install -r requirements.txt, these failures are now warnings:

$ pytest 

tests/default_cache_test.py ..                                                                                                                                 [  5%]
tests/details_test.py ....                                                                                                                                     [ 15%]
tests/handler_async_test.py ................                                                                                                                   [ 55%]
tests/handler_test.py ................                                                                                                                         [ 95%]
tests/init_test.py ..                                                                                                                                          [100%]

========================================================================== warnings summary ==========================================================================
tests/handler_async_test.py::test_get_batch_details[None]
tests/handler_async_test.py::test_get_batch_details[1]
tests/handler_async_test.py::test_get_batch_details[2]
tests/handler_async_test.py::test_get_batch_details[3]
tests/handler_async_test.py::test_get_batch_details_total_timeout[None]
tests/handler_async_test.py::test_get_batch_details_total_timeout[1]
tests/handler_async_test.py::test_get_batch_details_total_timeout[2]
tests/handler_async_test.py::test_get_batch_details_total_timeout[3]
  /Users/ipinfo/Desktop/python/ipinfo/handler_async.py:258: DeprecationWarning: The explicit passing of coroutine objects to asyncio.wait() is deprecated since Python 3.8, and scheduled for removal in Python 3.11.
    _, pending = await asyncio.wait(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================================================================== 40 passed, 8 warnings in 6.13s ===================================================================

I tested your changes and all looks good now:

$ pytest

tests/default_cache_test.py ..                                                                                                                                 [  4%]
tests/details_test.py ....                                                                                                                                     [ 12%]
tests/handler_async_test.py ....................                                                                                                               [ 54%]
tests/handler_test.py ....................                                                                                                                     [ 95%]
tests/init_test.py ..                                                                                                                                          [100%]

========================================================================== warnings summary ==========================================================================
tests/handler_async_test.py::test_get_details_error[5xx_not_json]
tests/handler_async_test.py::test_get_details_error[4xx_json]
tests/handler_async_test.py::test_get_details_error[400]
tests/handler_async_test.py::test_get_details_quota_error
  /Users/ipinfo/.pyenv/versions/3.10.14/lib/python3.10/site-packages/aiohttp/client.py:1156: RuntimeWarning: coroutine 'MockResponse.release' was never awaited
    self._resp.release()
  Enable tracemalloc to get traceback where the object was allocated.
  See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.

tests/handler_async_test.py::test_get_batch_details[None]
tests/handler_async_test.py::test_get_batch_details[1]
tests/handler_async_test.py::test_get_batch_details[2]
tests/handler_async_test.py::test_get_batch_details[3]
tests/handler_async_test.py::test_get_batch_details_total_timeout[None]
tests/handler_async_test.py::test_get_batch_details_total_timeout[1]
tests/handler_async_test.py::test_get_batch_details_total_timeout[2]
tests/handler_async_test.py::test_get_batch_details_total_timeout[3]
  /Users/ipinfo/Work/ipinfo_public/ipinfo-python/ipinfo/handler_async.py:262: DeprecationWarning: The explicit passing of coroutine objects to asyncio.wait() is deprecated since Python 3.8, and scheduled for removal in Python 3.11.
    _, pending = await asyncio.wait(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================== 48 passed, 12 warnings in 6.33s ===================================================================

[max-ipinfo]

Changes look good and tests pass with Python 3.10.

Thanks again @PriOliveira !

[PriOliveira]

I'm glad that in the end you figured out that it needs Python 3.10. Sorry that I wasn't there to point that out earlier.

Thanks a lot for the review, @max-ipinfo :)