Skip to content

handle status_code!=200 in client_refresh#134

Closed
mjj4791 wants to merge 6 commits into
picklepete:masterfrom
mjj4791:patch-1
Closed

handle status_code!=200 in client_refresh#134
mjj4791 wants to merge 6 commits into
picklepete:masterfrom
mjj4791:patch-1

Conversation

@mjj4791
Copy link
Copy Markdown

@mjj4791 mjj4791 commented Jun 6, 2017

Occasionally the request/POST to the refresh-url will return a http status=450 (i.e. the post failed, no json gets returned).
Without catching the non-200 status code, the call to req.json() will fail (causing a ValueError), since no json got returned.

With this proposed change, only when a 200 status code is returned, will the json get parsed;
in all other cases, a API response error gets raised.

mjj4791 added 2 commits June 6, 2017 23:46
Occasionally the request/POST to the refresh-url will return a http status=450 (i.e. the post failed, no json gets returned).
Without catching the non-200 status code, the call to req.json() will fail (causing a ValueError), since no json got returned.

With this proposed change, only when a 200 status code is returned, will the json get passed; 
in all other cases, a API response error gets raised.
fix pep8 errors
Copy link
Copy Markdown
Collaborator

@torarnv torarnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, just a small nit on the code flow

Comment thread pyicloud/services/findmyiphone.py Outdated

if not self._devices:
raise PyiCloudNoDevicesException()
else:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use early exit/raise

@torarnv
Copy link
Copy Markdown
Collaborator

torarnv commented Jun 7, 2017

Actually, come to think of it, we should probably detect this in base.py and raise from there, can you try that?

@mjj4791
Copy link
Copy Markdown
Author

mjj4791 commented Jun 7, 2017

Not sure what your intention is....

Do you mean to modify base.py as well?

  1. i.e. catch the newly added PyiCloudAPIResponseError in base.py (PyiCloudService.devices() ) and from there raise a PyiCloudNoDevicesException?
    pro: status code can be encapsulated in the exception

  2. OR, undo the changes in findmyiphone.py, but catch the ValueError in base.py and raise a PyiCloudNoDevicesException?
    con: you will not know what the cause was, status_code is lost

  3. OR: leave as proposed; raising PyiCloudAPIResponseError from the FindMyiPhoneServiceManager.refresh_client()?
    pro: raised as close to the cause of the error,returning actual reason & http status code returned...

either way; either solution this will allow calling applications to catch an exception and act accordingly....
let me know what your preference is....

mjj4791 added 4 commits June 11, 2017 20:45
early raise/exit
catch PyiCloudAPIResponseError and re raise as PyiCloudNoDevicesException
fixed trailing spaces
handle status_code!=200 in client_refresh
@mjj4791
Copy link
Copy Markdown
Author

mjj4791 commented Jun 23, 2017

@torarnv is the current PR acceptable?

@torarnv
Copy link
Copy Markdown
Collaborator

torarnv commented Jun 29, 2017

Hey, sorry for the delay!

I'm a bit curious about the HTTP response, does the server return any content at all? An error message? HTTP 450 is "Blocked by Windows Parental Controls (Microsoft)", but it's not standardized so Apple could in theory use it for something completely different.

Anyways, I've pushed a #138 to try to solve this, but it's completely untested. Can you try it to see if it helps?

@torarnv
Copy link
Copy Markdown
Collaborator

torarnv commented Jul 8, 2017

Fixed in #138

@torarnv torarnv closed this Jul 8, 2017
@mjj4791 mjj4791 deleted the patch-1 branch August 30, 2017 21:45
jacobhp-heni pushed a commit to jhads/pyicloud that referenced this pull request Nov 27, 2025
* Add logging for device refresh and response handling in FindMyiPhoneService

* Fix refresh client reauth test to handle additional exception case

* Enhance reauthentication logic in FindMyiPhoneServiceManager to support retry mechanism
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants