watchforstock / evohome-client

Python client to access the Evohome web service
Apache License 2.0
88 stars 52 forks source link

Improve exception handling during _basic_login() #91

Closed zxdavb closed 5 years ago

zxdavb commented 5 years ago

A proposed fix to issue #85

Both versions of EvohomeClient should behave consistently when handling requests exceptions and when producing error/warning messages, providing useful feedback where possible.

One core issue is that raise_for_exception() does not include response.text (or the JSON equivalent) - this is by design.

Another issue is that the RESTful API is undocumented, so we should be mindful that responses may not be as expected from (say) a review of OAuth documentation..

Basic approach: a) catch all authentication/authorization/session expiry errors & attempt to transparently fall back to re-authenticate (access_token -> refresh_token -> user credentials), raiseing only if that fails (e.g. bad user credentials) b) catch all HTTPErrors to test if they have a response (usu. JSON), and subsequently raise them, including that message c) (blindly) raise anything else - using raise_for_exception()

Raising a ValueError or a KeyError is not consistent with that, but these exist a final checks of validity.

zxdavb commented 5 years ago

OK, I have put a custom exception, AuthenticationError, back in:

try:
    response.raise_for_status()
except requests.HTTPError:
    msg = "Unable to obtain an Access Token"
    if response.text:
        msg = msg + ", hint: " + response.text
    raise AuthenticationError(msg)

Wrapping raise_for_status() in a try block is a simple solution, that shows both the requests.exceptions.HTTPError (without modification), and the consequence "Unable to obtain an Access Token", and a hint (usu. response.text):

Traceback (most recent call last):
  File "/home/dbonnes/home-assistant/venv/lib/python3.6/site-packages/evohomeclient2/__init__.py", line 146, in _obtain_access_token
    response.raise_for_status()
  File "/home/dbonnes/home-assistant/venv/lib/python3.6/site-packages/requests/models.py", line 940, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: https://tccna.honeywell.com/Auth/OAuth/Token

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/dbonnes/home-assistant/venv/lib/python3.6/site-packages/evohomeclient2/__init__.py", line 198, in user_account
    response = requests.get(url, headers=self._headers())
  File "/home/dbonnes/home-assistant/venv/lib/python3.6/site-packages/evohomeclient2/__init__.py", line 87, in _headers
    self._basic_login()
  File "/home/dbonnes/home-assistant/venv/lib/python3.6/site-packages/evohomeclient2/__init__.py", line 111, in _basic_login
    self._obtain_access_token(credentials)
  File "/home/dbonnes/home-assistant/venv/lib/python3.6/site-packages/evohomeclient2/__init__.py", line 152, in _obtain_access_token
    raise AuthenticationError(msg)
evohomeclient2.AuthenticationError: Unable to obtain an Access Token, hint: {"error":"invalid_grant"}

I also got rid of the rather unsatisfactory ValueError, KeyError in favour of AuthenticationError (in a architecturally identical way):

except KeyError:
    raise AuthenticationError("Unable to obtain an Access Token, hint: " + response_json)
except ValueError:
    raise AuthenticationError("Unable to obtain an Access Token, hint: " + response.text)
zxdavb commented 5 years ago

This code runs without error in my testbed (using Home Assistant).