robocjk.api.client.Client doesn't raise errors on HTTP errors about django-robo-cjk HOT 11 CLOSED

Btw. the server returns "400 Bad Request" if the user name or password is wrong in the "api/auth/token/" call.

I think the best solution is that client._api_call() should raise an error upon status_code != 200, then client.auth_token() should catch that and turn it into an error that says something about incorrect credentials.

from django-robo-cjk.

Come to think of it, what happens if you get a 401 on a 401? Is there a chance of a runaway recursion with the way this code is currently written? _api_call is calling itself after all.

from django-robo-cjk.

@justvanrossum

• About the error policy, actually it is possible to check the response status code, but raising an Exception would be surely more safe than possible silent errors, we could add a silent_errors=True option to the constructor just to offer backward compatibility to the current implementation.

• This is technically impossible, because the API called for obtaining the auth token doesn't require authorization, so the 401 error (unauthorized) cannot be raised. If we want to discuss about the recursion, neither me I like it, but for now I think it is ok.

from django-robo-cjk.

Is there a case where the status_code is anything else but a 200 and things actually keep on working?

What I observe now is that a wrong username results in AttributeError: 'NoneType' object has no attribute 'get' a bit later, which is 100% unhelpful :)

from django-robo-cjk.

Also: backwards compatible from whom? We are the only users, no?

from django-robo-cjk.

I don't know if the response code is always checked or not in the client implementation.

The wrong username error seems strange, it should be surely improved. To reproduce it, is it enough to initialise the client with an invalid username? If not, could you tell me how to reproduce it, please?

Backward compatibility for our current implementation.

from django-robo-cjk.

I don't know if the response code is always checked or not in the client implementation.

Anything not 200 is an error, and therefore means you're not getting the response you hoped for. You cannot simply proceed and expect anything to work.

The wrong username error seems strange, it should be surely improved. To reproduce it, is it enough to initialise the client with an invalid username?

Yes, that's all there's to it. And it's directly caused by not raising an error earlier upon a non-200 response.

Backward compatibility for our current implementation.

Which code relies on a response other than 200, and actually works? My guess is: none whatsoever.

The RoboCJK RF extension just treats any error as "wrong credentials", which is also a symptom of this problem:
https://github.com/BlackFoundryCom/robo-cjk/blob/2e73fbb475a02ff6484d2c712d8c3a622ad204dd/sources/views/sheets.py#L885-L891

from django-robo-cjk.

API error responses already contain the error message, so we can just raise an exception using that message.

from django-robo-cjk.

Ah yes, this works nicely:

--- a/client.py
+++ b/client.py
@@ -7,6 +7,10 @@ import urllib3
 urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
 
 
+class HTTPError(Exception):
+    pass
+
+
 class Client(object):
 
     """
@@ -107,6 +111,9 @@ class Client(object):
                 return self._api_call(view_name, params)
         # read response json data and return dict
         response_data = response.json()
+        if response.status_code != 200:
+            raise HTTPError(f"{response.status_code} {response_data['error']}")
+
         return response_data

from django-robo-cjk.

I can make a PR if you want, after #5.

from django-robo-cjk.

Sure, PR would be appreciated!

from django-robo-cjk.