Multiple errors thrown on reaching max retries by #580

[Quizz1Cal]

NOTE: My first issue to a public repo, any feedback is appreciated :)

Describe the bug
Using spotipy post-#580 (relevant issue #571 ) yields uncaught urllib3 / requests library errors, and in particular an AttributeError due to retry_error.response being None (see below). Furthermore, though not featured below, line 274 of client.py under this commit will also throw an AttributeError for the same reason.

Your code
Note: Have defined environment variables SPOTIPY_CLIENT_ID, SPOTIPY_CLIENT_SECRET, SPOTIPY_CLIENT_USERNAME and SPOTIPY_REDIRECT_URI.

# GetClientCredentials() also works here, the playlist is public
spotify = Spotify(auth_manager=SpotifyOAuth(scope='playlist-read-private'))
brain_food_playlist_id = '37i9dQZF1DWXLeA8Omikj7'
playlist = spotify.playlist(playlist_id=brain_food_playlist_id)
print(spotify.playlist_is_following(playlist_id=playlist['id'], user_ids=['spotify:user:bad_username']))

Expected behavior
According to the implementation of the requests library, responses are expected to not be passed to RetryErrors (see source code here; AFAIK this is the only circumstance it is thrown by the library).

Therefore, it is my belief that:

1. A SpotifyException should be thrown (potentially, with the associated url and header, if retrievable)
2. (if would not unreasonably impact existing behaviour) The non-spotipy exceptions should not be thrown

Output
WARNING: Paths modified for privacy

Max Retries reached
Traceback (most recent call last):
  File "../project/venv/lib/python3.8/site-packages/requests/adapters.py", line 439, in send
    resp = conn.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 817, in urlopen
    return self.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 817, in urlopen
    return self.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 817, in urlopen
    return self.urlopen(
  File "../project/venv/lib/python3.8/site-packages/urllib3/connectionpool.py", line 807, in urlopen
    retries = retries.increment(method, url, response=response, _pool=self)
  File "../project/venv/lib/python3.8/site-packages/urllib3/util/retry.py", line 439, in increment
    raise MaxRetryError(_pool, url, error or ResponseError(cause))
urllib3.exceptions.MaxRetryError: HTTPSConnectionPool(host='api.spotify.com', port=443): Max retries exceeded with url: /v1/playlists/37i9dQZF1DWXLeA8Omikj7/followers/contains?ids=spotify:user:bad_fakename (Caused by ResponseError('too many 500 error responses'))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 239, in _internal_call
    response = self._session.request(
  File "../project/venv/lib/python3.8/site-packages/requests/sessions.py", line 530, in request
    resp = self.send(prep, **send_kwargs)
  File "../project/venv/lib/python3.8/site-packages/requests/sessions.py", line 643, in send
    r = adapter.send(request, **kwargs)
  File "../project/venv/lib/python3.8/site-packages/requests/adapters.py", line 507, in send
    raise RetryError(e, request=request)
requests.exceptions.RetryError: HTTPSConnectionPool(host='api.spotify.com', port=443): Max retries exceeded with url: /v1/playlists/37i9dQZF1DWXLeA8Omikj7/followers/contains?ids=spotify:user:bad_fakename (Caused by ResponseError('too many 500 error responses'))

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "../project/src/issue_boilerplate.py", line 10, in <module>
    print(spotify.playlist_is_following(playlist_id=playlist['id'], user_ids=['spotify:user:bad_fakename']))
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 1145, in playlist_is_following
    return self._get(
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 286, in _get
    return self._internal_call("GET", url, payload, kwargs)
  File "../project/venv/lib/python3.8/site-packages/spotipy/client.py", line 273, in _internal_call
    "%s:\n %s" % (response.url, "Max Retries"),
AttributeError: 'NoneType' object has no attribute 'url'

Environment:

• OS: Ubuntu 20.04.1 LTS
• Python version: 3.8.2
• spotipy version: Commit 18e82b6 (Fixed UnboundLocalError #571 #580 )
• your IDE: xterm

[stephanebruckert]

Hi @Quizz1Cal, thanks for the nice report.

In your opinion would reverting #580 and re-opening #571 help? Also if you feel confident to write a fix for this, that would be really appreciated 🙏

[Quizz1Cal]

In the sense that #580 does not spiritually resolve #571, perhaps a revert would be best.

In terms of a fix, I'm basically already there so going the full distance should be fine! It comes down to what you'd want to see in a thrown SpotifyException. I can extract the true url from https://api.spotify.com/{v1/etc...} from the error.request.url, which I presume would always equal to response.url. Shouldn't be hard to catch the other exceptions, but can you confirm it would be appropriate to catch or otherwise avoid throwing the urllib3 / requests library errors?

If you approve the PR, would you be willing to label it as hacktoberfest-accepted? (It's not the reason I'm using spotipy, so I'll still fix it regardless :) )

[Quizz1Cal]

I've developed a fix:

# ... spotipy/client.py line 267
except requests.exceptions.RetryError as retry_error:
    request = retry_error.request
    logger.error('Max Retries reached')
    try:
        reason = retry_error.args[0].reason
    except (IndexError, AttributeError):
        reason = None
    raise SpotifyException(
        599,
        -1,
        "%s:\n %s" % (request.path_url, "Max Retries"),
        reason=reason
    )

I have only attempted action (1) from above; it should be noted that catching the SpotifyException itself here will silence all aforementioned errors.

I'm not sure if I should open a PR if you haven't yet reverted #580 , so I figure I'd post the minor change here for now. It passes all tests, though I don't believe we have any tests to deal with this specific issue. Perhaps something to add?

[stephanebruckert]

In the sense that #580 does not spiritually resolve #571, perhaps a revert would be best.

I reverted #580 in #582 and reopened the initial issue #571. In your PR, feel free to solve both #571 and #581

If you approve the PR, would you be willing to label it as hacktoberfest-accepted?

Definitely!

can you confirm it would be appropriate to catch or otherwise avoid throwing the urllib3 / requests library errors

Your fix looks good, displaying retry_error.request.path_url as part of a SpotifyException totally makes sense.

It passes all tests, though I don't believe we have any tests to deal with this specific issue. Perhaps something to add?

Yes integration tests might not consistently test this. If you find a way to unit test/mock it, that'd be amazing, otherwise feel free to submit the PR without it. On my side I'll be testing this manually to make sure it's all good.