Detect if the login fails and throw appropriate exception

[andrew-blake]

PR to detect if a login 403 error and throw a more obvious exception message than a JSON decode error. Helps address issue https://github.com/indykoning/PyPi_GrowattServer/issues/49

[indykoning]

Thank you! i agree that this is something that should be addressed because the current messages are not clear. I'd prefer to leave the error messages to the implementation of this package though.

While reviewing i found response.raise_for_status() https://requests.readthedocs.io/en/latest/user/quickstart/#response-status-codes This seems like the best solution to me, since it will throw the proper exception for the different cases automatically. Thus applications implementing this package can show their own customized, translated messages for the different error codes

[andrew-blake]

I hadn't noticed response.raise_for_status() before - that will come in useful elsewhere, thanks.

I've updated the PR to use it (forced pushed).

I've noticed Growatt have updated their WAF to block the default User Agent, so I think it makes sense to add a specific GrowattAPI exception for that. I've proposed growattServer.GrowattApiUserAgentBlockedException, with the generic requests.exceptions.HTTPError for all other HTTP level issues.

Also updated the PR to detect GrowattApiIncorrectPasswordException and GrowattApiUnknownException for unknown JSON responses.

[indykoning]

Is it okay if i implement the raise_for_status, and think about the other exceptions a little longer?

I'm afraid of losing context for the developer using it instead of gaining it. E.g. https://github.com/home-assistant/core/blob/dev/homeassistant/components/growatt_server/config_flow.py#L60 would need to update their implementation of checking login status, however they wouldn't be able to check the return message anymore in the exception.

I think the raise_for_status will not cause a breaking change (If it will not raise a http exception it will raise a json decode exception) but the check for success will

[muppet3000]

Is it okay if i implement the raise_for_status, and think about the other exceptions a little longer?

I'm afraid of losing context for the developer using it instead of gaining it. E.g. https://github.com/home-assistant/core/blob/dev/homeassistant/components/growatt_server/config_flow.py#L60 would need to update their implementation of checking login status, however they wouldn't be able to check the return message anymore in the exception.

I think the raise_for_status will not cause a breaking change (If it will not raise a http exception it will raise a json decode exception) but the check for success will

I really like this approach @indykoning - Good catch on the Home Assistant implementation. I think throwing the exception for users of the library to handle is a really good idea. It should also mean that we can handle the scenarios when the servers are unavailable in a more consistent manner as well.

[marcovtwout]

@indykoning Could you implement the basic raise_for_status as you suggested? That might make future debugging easier as for example HTTP 405 occurs more and more often.

[marcovtwout]

If I understand correctly raising exceptions was already implemented in 1.3.1 and this PR should be closed: https://github.com/indykoning/PyPi_GrowattServer/commit/667a520e1cb7ee7e22fdbc695c5f6173fb2276a2