retrieve proper message and reason when response can't be loaded to json

[gustavokrieger]

I couldn't reproduce the throwing of BrokenPipeError reported in #633, but noticed that the SpotifyException that was being thrown instead had None as reason. I then noticed that both msg and reason could possibly be retrieved from the response's raw, so I created a method to do it (only if loading to json fails). I also extracted a method from the original code.

[Peter-Schorn]

The requests documentation for the raw property of Response says:

#: File-like object representation of response (for advanced usage).
#: Use of raw requires that stream=True be set on the request.
#: This requirement does not apply for use internally to Requests.
self.raw = None

But stream is not set to True on the request. Have you tested this?

[gustavokrieger]

Oh, nice catch. I'll look into it.

[Peter-Schorn]

You should just return the text of the response if it can't be converted to JSON. Raw data is unlikely to be useful and I can't image a scenario where spotify returns data that can't be converted to a string.

[gustavokrieger]

I see that that there is a reason property in the Response. Maybe use that as reason, and use the text property as msg (if empty use 'error'). What do you think?

[Peter-Schorn]

reason is just a string describing the status code, which is already provided to the initializer for SpotifyException, so I think this is redundant.

[Peter-Schorn]

Furthermore, the reason property corresponds to the reason field of the player error object. If Spotify doesn't return a player error, then reason should be None.

[Peter-Schorn]

Here's a much simpler solution: Just change this line from
https://github.com/plamere/spotipy/blob/c4f6a3fa4b4e5e13d78594c33874a0e6cb18361e/spotipy/client.py#L252
to

msg = response.text

[gustavokrieger]

Ok, just to make sure we are on the same page: the response has a reason, but isn't being added to the SpotifyException because the json() method fails, causing the reason to receive None from:

https://github.com/plamere/spotipy/blob/c4f6a3fa4b4e5e13d78594c33874a0e6cb18361e/spotipy/client.py#L256

[gustavokrieger]

Debugging before the json conversion error (notice that the reason could be retrieved at this moment):

[gustavokrieger]

The stack trace itself (notice how the HTTPError shows the reason "Request Entity Too Large" but SpotifyException does not):

HTTP Error for PUT to https://api.spotify.com/v1/playlists/1fYfHp88zjW0kVZItm20gu/images returned 413 due to error
Traceback (most recent call last):
  File "D:\Users\gustavo\PycharmProjects\spotipy\spotipy\client.py", line 245, in _internal_call
    response.raise_for_status()
  File "D:\Users\gustavo\PycharmProjects\spotipy\venv\lib\site-packages\requests\models.py", line 943, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 413 Client Error: Request Entity Too Large for url: https://api.spotify.com/v1/playlists/1fYfHp88zjW0kVZItm20gu/images

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:/Users/gustavo/PycharmProjects/spotipy/t.py", line 46, in <module>
    upload()
  File "D:/Users/gustavo/PycharmProjects/spotipy/t.py", line 14, in upload
    spotify.playlist_upload_cover_image(playlist_id, encoded_image)
  File "D:\Users\gustavo\PycharmProjects\spotipy\spotipy\client.py", line 697, in playlist_upload_cover_image
    content_type="image/jpeg",
  File "D:\Users\gustavo\PycharmProjects\spotipy\spotipy\client.py", line 306, in _put
    return self._internal_call("PUT", url, payload, kwargs)
  File "D:\Users\gustavo\PycharmProjects\spotipy\spotipy\client.py", line 266, in _internal_call
    headers=response.headers,
spotipy.exceptions.SpotifyException: http status: 413, code:-1 - https://api.spotify.com/v1/playlists/1fYfHp88zjW0kVZItm20gu/images:
 error, reason: None

[gustavokrieger]

And instead of retrieving the reason and adding it to the SpotifyException, it should only set the response.text to msg?

[Peter-Schorn]

Ok, just to make sure we are on the same page: the response has a reason, but isn't being added to the SpotifyException because the json() method fails, causing the reason to receive None from:

https://github.com/plamere/spotipy/blob/c4f6a3fa4b4e5e13d78594c33874a0e6cb18361e/spotipy/client.py#L256

Yes, precisely. The instance of Response returned by
https://github.com/plamere/spotipy/blob/c4f6a3fa4b4e5e13d78594c33874a0e6cb18361e/spotipy/client.py#L240-L243
will, of course, have a reason property, because, again, this is just a string describing the http status code.

The reason property of SpotifyException, on the other hand, is meant to correspond to the reason field of the player error object. Just because these properties have the same name doesn't mean they represent the same thing.

[Peter-Schorn]

Notice that SpotifyException does have a http_status property. You can use this to get a string describing the status code. Take a look at https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

[gustavokrieger]

Now I get it. Thanks for the explanation 🙌

Now, how about:

msg = response.text if response.text else "error"

Instead of:

msg = response.text

in:

https://github.com/plamere/spotipy/blob/c4f6a3fa4b4e5e13d78594c33874a0e6cb18361e/spotipy/client.py#L252

To avoid an empty msg?

[Peter-Schorn]

response.text may raise an exception (instead of returning None) if the data cannot be decoded into a string. You'll have to test this.