felix-hilden / tekore Goto Github PK

Should access tokens be handled in some higher-level object?

Currently Credentials provides methods for requesting and refreshing access tokens. However, no way of automatically refreshing tokens. It's surely a common use case and easy to implement.

Refreshing token appears to never expire. However, its properties related to expiration still delegate to the underlying expiring token. Should they be

• expires_in = None
• expires_at = None
• is_expiring = False

instead?

Creating, reading and modifying collaborative playlists requires a combination of playlist-modify-public and playlist-modify-private scopes. And for example the public attribute must be set to false. Document this behavior in functions and create a tutorial or documentation section dedicated to collaborative playlists.

A client sends requests to the Spotify API and returns objects that could be parsed to models. To which types should the involved functions be separated?

Models stand on their own. A fine-grained separation would involve a pure client with no knowledge of specific endpoints and a class for constructing requests and parsing responses. But they could very well be one. If the caller has a reason not to have the response automatically parsed or it is just better design, they could be separated.

Spotify API provides headers for client-side caching. Use them. Documentation link.

spotipy/client/__init__.py", line 118, in current_user return PrivateUser(**json) TypeError: __init__() got an unexpected keyword argument 'birthdate'
seems birthdate not mentioned on current documentation. It looks like: "birthdate": "2019-10-19"

I'm planning on porting a private project to this new version of Spotify. Previously mine was in Python 2.7. I find the new functionality encouraging, although could you elucidate with a few examples so I can see how they it might be used?

Should the conversion inputs be validated? The operations themselves are simple, but can yield nonsensical results if called with bad inputs.

For example: track:spotify:xyz from URI would give type: spotify, id: xyz and type: open.spotify.com/track/xyz, id: xyz to URL would give http://open.spotify.com/open.spotify.com/track/xyz/xyz

example playlist: spotify:playlist:6fzw3GiLktfRKczD6sl6mT
bug: spotify/web-api#958

Traceback:
File "spotipy/model/playlist.py", line 33, in post_init
self.items = [PlaylistTrack(**t) for t in self.items]
File "spotipy/model/playlist.py", line 33, in
self.items = [PlaylistTrack(**t) for t in self.items]
File "", line 8, in init
File "spotipy/model/playlist.py", line 24, in post_init
self.track = FullTrack(**self.track)
TypeError: type object argument after ** must be a mapping, not NoneType

on artist_albums() with market='from_token', but not with market=<country_code>

... should be as str?

Related to #64. Currently None fields are hinted with field: type = None. This is a bit incorrect, because it implies that the fields are sometimes not returned at all. These should be changed to field: Optional[type] where possible, and if not, document why the presence of the attribute is not known.

Track audio analysis and playback information calls return responses that are not documented as objects in the Web API. These responses are now returned as dicts. Determine if they always have set members. If they do, make Models for them.

Having documentation available in Read The Docs could be very beneficial. Investigate what is needed and what could be the best workflow of building the documentation, perhaps it could be done in continuous integration.

Currently waiting for the decision on project name in #22. If the transfer is successful, an issue to RTD should be opened to transfer the documentation name as well.

This model is apparently missing actions. You can see it in the official docs. Not really sure if the models part is completely finished but I wanted to let you know.

    metadata = self._spotify.playback_currently_playing()
  File "/home/mario/.local/lib/python3.7/site-packages/spotipy/client/player.py", line 49, in playback_currently_playing
    return CurrentlyPlaying(**json)
TypeError: __init__() got an unexpected keyword argument 'actions'

E: whoops, I probably misread this message, sorry about that. Anyway, great job on the models :)

When I looked up the RefreshingToken entry in the docs, I didn't find much about how it works. Right now, you can do something like this:

regular_token = creds.request_user_token(code, scope)
refreshing_token = RefreshingToken(regular_token, creds)

But this different implementation

data = {
    'access_token': auth_token,
    'token_type': 'Bearer',
    'scope': Scope(scopes.user_read_currently_playing),
    'expires_in': expiration - int(time.time())
}
creds = Credentials(client_id, client_secret, redirect_uri)
token = RefreshingToken(Token(data), creds)

doesn't work because apparently refresh_token also has to be set in the Token data:

  ...
  File "/home/glow/Downloads/spotify-music-videos/spotivids/api/web.py", line 80, in _refresh_metadata
    metadata = self._spotify.playback_currently_playing()
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/client/player.py", line 58, in playback_currently_playing
    json = self._get('me/player/currently-playing', market=market)
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/client/base.py", line 128, in _get
    return self._request('GET', url, payload=payload, params=params)
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/client/base.py", line 122, in _request
    request = self._build_request(method, url)
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/client/base.py", line 75, in _build_request
    'Authorization': f'Bearer {self.token}',
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/client/base.py", line 45, in token
    return str(self._token)
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/auth.py", line 43, in __str__
    return self.access_token
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/util.py", line 39, in access_token
    self._token = self.credentials.refresh(self._token)
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/auth.py", line 243, in refresh
    return self.request_refreshed_token(token.refresh_token)
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/auth.py", line 222, in request_refreshed_token
    refreshed = request_token(self._auth, payload)
  File "/home/glow/.local/lib/python3.7/site-packages/spotipy/auth.py", line 93, in request_token
    content['error_description']
spotipy.auth.OAuthError: 400 invalid_request: refresh_token must be supplied

This refresh_token is returned with request_user_token, right? Or how does it work? If I understand it correctly, the first example works but the second doesn't because it's missing refresh_token? It'd be a good idea to include this in the docs.

Many endpoints that require user authentication are prefixed by current_user. Others, like playlist and playback endpoints don't have that prefix. The naming should be consistent.

Having the prefix in every endpoint would be consistent but would also lead to impossibly long method names like current_user_playlist_cover_image_upload. And with some endpoints, like the playback ones, it is quite obvious that they require user authentication.

I think if there is no ambiguity whether a user account is needed, the prefix should be removed. And if it leads to bad method names, they should be renamed. But there is some value in keeping names consistent across an API.

By API:

• Album, Artist, Browse, Track: no user authentication needed
• Follow: actions like follow and unfollow, or getting followed_artists are clear, but e.g. artists_is_following would be a bit confusing
• Library: current_user could be replaced with saved (like saved_albums), which would have the advantage of being able to discover the library API more easily
• Player: unambiguously related to a user
• Playlist: unambiguously related to a user or a shared playlist, except current_user_playlists
• Misc: top artists and tracks are ambiguous, they could retain the prefix

If we want to be cute about it, we could even check if the current token is a user token and throw an error early. This could be implemented as a simple decorator on every required method.

As you created some issues to track some features and ideas, I thought this would be useful too. plamere/spotipy has plenty of tests we could clean and maybe refactor to suit this API. Also, a continuous integration bot like Travis CI could help with these tests. What do you think?

I think its pretty common use case.

I currently use:

    def all_pages_from_paging(self,
                              paging: Paging
                              ) -> Generator[Paging, None, None]:
        '''all pages from Paging generator'''
        yield paging
        while paging.next is not None:
            paging = self.next(paging)
            yield paging

    def all_items_from_paging(self,
                              paging: Paging
                              ) -> Generator[Item, None, None]:
        '''all items from Paging generator'''
        for page in self.all_pages_from_paging(paging):
            yield from page.items

and:

paging = spotify.playlists('thesoundsofspotify')
print(paging.total)
for playlist in spotify.all_items_from_paging(paging):
     print(playlist.name)

prompt_for_user_token in the util module retrieves automatically refreshing user tokens. Client tokens expire too, so they should have a similar function. Implement a RefreshingCredentials class to serve the purpose of always returning refreshing tokens.

It would be convenient if the str representation of Models would be the equivalent JSON, or at least the class could be passed to json.dumps. They would then be represented by the equivalent string that was returned from a request from the Web API.

Duplicating information is no good, and it leads to harder-to-maintain documentation. Having the most relevant information in correct places and linking between them could be a better strategy than just having one readme for all services surrounding the package.

Here's what I'm currently at. Each readme would include a short section with general information. It would be pretty much the same for every document. It would introduce spotipy and link to other services. It could also include a short example snippet of code. Then every readme would have some service-specific information.

GitHub

• Cloning the repository
• Running tests
• Building documentation
• Contributing

Read The Docs

• Features
• Getting started (with the Web API application as well)
• Documentation structure

PyPI

• Package info and history (all the relevant information regarding previous versions)
• How to install (possibly using different methods along with pip)

Maybe something else too.

on playback_recently_played() .. next()

Traceback:
 ...
  File "spotipy/client/base.py", line 165, in next
    return type(result)(**next_set)
  File "<string>", line 7, in __init__
  File "spotipy/model/play_history.py", line 34, in __post_init__
    self.cursors = PlayHistoryCursor(**self.cursors)
TypeError: type object argument after ** must be a mapping, not NoneType

probably on last page. api returns:

{
  "items": [],
  "next": null,
  "cursors": null,
  "limit": 20,
  "href": "https://api.spotify.com/v1/me/player/recently-played?before=1573451743204"
}

It would be convenient for users to be able to import things straight from the spotipy level instead of submodules. What should be importable that way?

Currently the imports are:

• auth: Token, Credentials
• scope: Scope, scopes
• client: Spotify

I see two approaches, make only the bare necessities importable or make everything that is useful to the user importable. The bare necessities would in my opinion only involve Credentials and Spotify, as they enable retrieving a token and calling the API. Everything useful for the user would include:

• client: Spotify
• model: none from here
• auth: Credentials
• convert: to and from URL and URI
• scope: Scope, scopes, read, write and every
• sender: all senders
• serialise: none from here
• util: read env, creds from env, prompt and refresh

But this could be confusing. In my opinion the better thing to do is just have Credentials and Spotify at the top level and have everything else in their modules. That way they would be grouped nicely.

https://developer.spotify.com/documentation/general/guides/local-files-spotify-playlists/

Since the required version is python 3.7, you could consider implementing f-strings in some parts of the module. It's a small thing but I've seen it several times while looking at your code and I think it could help readibility, like in this example.

return 'http status: {0}, code:{1} - {2}'.format(
    self.http_status, self.code, self.msg)

could be replaced with

return f'http status: {self.http_status}, code:{self.code} - {self.msg}'

I can do a PR myself if you want, but I wanted to ask what you think about it first.

It would be convenient to be able to define scopes with plain strings as well. Accept string whenever a Scope object is accepted. In addition, accept list of strings or space-separated string in constructor to Scope.

Method is currently a no-op. Implement functionality:

SpotifyPlaylist.playlist_cover_image_upload(self, playlist_id: str, image: str)

where image is a base64-encoded jpg image of size 256 kB or less. The content type must be set to image/jpg and the request sent to playlists/id/images (PUT).

undocumented

... but search accepts 'episode' and 'show' type
... and self._get('episodes/' + episode_id, market=market) just like tracks

Found 75 errors in 23 files (checked 43 source files)

I have noticed a few cases where it could be useful to implement higher-level methods. With this issue I'd like to gather ideas about such methods, assess whether it would be worth it to implement them and how they should be implemented.

• Restore playback from playback context / playback start for objects (like Album)
• Change playlist details with a playlist object (get, modify attributes, update)

This issue is in no hurry to close. This functionality would be extra.

.. for reducing needed api calls and speeding up thing

While they do a wonderful job at putting it in an obscure place, Spotify do state that refresh tokens may not be evergreen. In their "Authorization Guide, Authorization Code Flow" section, they write:

A token that can be sent to the Spotify Accounts service in place of an authorization code. (When the access code expires, send a POST request to the Accounts service /api/token endpoint, but use this code in place of an authorization code. A new access token will be returned. A new refresh token might be returned too.)

I haven't built the docs and I don't quite remember where in the repo docs it's mentioned, but I'm pretty sure there's a section there that at least implies that refresh tokens don't expire. Perhaps the phrasing should be changed?

A more important change to make is in the code, in case spotipy doesn't currently handle this kind of response (one that also possibly includes a refresh_token).

From #89. While I think it is up to the user to read the reference documentation or view the Web API documentation in order to learn all the available endpoints, the examples should be expanded to contain at least an example from every API. They should be a bit more interesting than a single call and printing out the results. Let's make them personalised too.

Here are some:

• Browse, Album: get album tracks of a new release
• Artist: get albums of a user's top artist, get related artists of a user's top artists and highlight ones that were already followed
• Browse, Follow: follow a spotify playlist in a category
• Misc, Follow: follow an artist by searching their name
• Misc, Browse, Playlist: create a playlist of recommendations based on a user's top tracks
• Playlist, Track: analyse a track in a user's playlist
• Library, Player: start playing a saved album

Traceback:
File "spotipy/client/base.py", line 129, in next
return type(result)(**self._get(result.next))
TypeError: init() got an unexpected keyword argument 'categories'

requests uses simplejson when available and only json.decoder.JSONDecodeError excepted on _parse_json

I was trying to use this API but I constantly get this error for some reason... The setup is quite minimal and it still happens:

from spotipy import Spotify, Scope, scopes, Token, Credentials
from spotipy.util import prompt_for_user_token, RefreshingToken


client_id = '...'
client_secret = '...'
redirect_uri = 'http://localhost:8888/callback/'
scope = Scope(*scopes)

token = prompt_for_user_token(client_id, client_secret, redirect_uri, scope)

spt = Spotify(token)


print(spt.current_user_albums())
print(spt.playback_currently_playing())

Could it be because I'm passing to Spotify a RefreshingToken instead of a Token? I'm 100% sure that my keys are correct.

Log:

❯ py test.py
Opening browser for Spotify login...
Please paste redirect URL: http://localhost:8888/callback/?code=...
Traceback (most recent call last):
  File "test.py", line 14, in <module>
    print(spt.current_user_albums())
  File "/home/mario/.local/lib/python3.7/site-packages/spotipy/client/library.py", line 25, in current_user_albums
    return self._get('me/albums', market=market, limit=limit, offset=offset)
  File "/home/mario/.local/lib/python3.7/site-packages/spotipy/client/base.py", line 71, in _get
    return self._send(r)
  File "/home/mario/.local/lib/python3.7/site-packages/spotipy/client/base.py", line 61, in _send
    raise HTTPError(f'Error ({r.status_code}) in {r.url}', request=r)
requests.exceptions.HTTPError: Error (400) in https://api.spotify.com/v1/me/albums?market=from_token&limit=20&offset=0

Edit: turns out I'm dumb. I should have used Spotify(self._token.access_token) instead of Spotify(self._token)

Some calls return models that are only documented with the calls. For example currently playing. Implement them as models.

example track: spotify.track('6F6CuSuM8EcD4UD0N3nuxN', market='SE')

Traceback:
File "spotipy/client/track.py", line 30, in track
return FullTrack(**json)
File "", line 24, in init
File "spotipy/model/track.py", line 66, in post_init
self.restrictions = Restrictions(**self.restrictions)
TypeError: init() got an unexpected keyword argument 'reason'

Some calls require specific scopes or types of credentials to function.

For example:

• Featured playlists can be retrieved with application credentials
• Searching requires user credentials
• Some fields of a user request are provided only with certain scopes
• Playlist and follow modifications generally require scopes

Determine the credentials and scopes required for each call. Document them. Should access be restricted before a request or by catching an error from Spotify?

What should be the way to release the package and its documentation?

In terms of branching, the releases could be implemented as tags. This way the master branch can be used as the default branch, not just the latest published version. Then tests would be run and documentation build verified on every merge. Releases to PyPI and RTD would be done by hand when the version is deemed ready. If the project grows, a separate development branch could be used. And if different configurations could be used in Travis, the master branch build could automatically publish the new version.

Not sure yet. Thoughts, anyone?

I'm creating this issue to keep track of this API progress. I know it's too early to release it but this is helpful for people who want to use it via PyPi in their project. It'd also need a new name, as I told you in the original spotipy thread.

Also, I wanted to let you know that once it's done I can take care of uploading and maintaining it in the AUR repos.

Do you think a __str__ method on classes like RefreshingToken would help? Instead of having to do s = Spotify(token.access_token), s = Spotify(token) would be enough and it'd make sense because there's not much more to do with a token.

Edit: my bad, s = Spotify(token) wouldn't actually call __str__ so this is kinda pointless. Sorry.

Currently error models are not used. Errors are raised with raw responses instead. Consider if using the models is beneficial. If so, implement the usage, otherwise delete the models.

Should timestamps be handled in some special way?

They are returned from the API in YYYY-MM-DDTHH:MM:SSZ zero-offset UTC. That precision may not be needed in displaying, but should be internally handled. The field could be an instance of datetime or perhaps a dataclass with an overrided __str__ method to produce the original format.

In the original API there was a very useful function called prompt_for_user_token in the util module that automatically got the access token and made the setup much easier. Currently, this API setup is something like this:

scope = Scope(scopes.user_read_currently_playing)
client = Credentials(self._client_id, self._client_secret, self._redirect_uri)
url = client.authorisation_url(scope)
webbrowser.open(url)
print("Please paste the URL you were redirected to in your browser: ")
code = input()
token = client.request_access_token(code, scope)
self._token = token.split("?code=")[1].split("&")[0]
self._spotify = Spotify(self._token)

compared to:

scope = 'user-read-currently-playing'
self._token = util.prompt_for_user_token(username, scope, client_id, client_secret, redirect_uri)
self._spotify = spotipy.Spotify(auth=self._token)

Now, I'm not sure if I'm using it correctly but there are a few problems here:

• Getting the scope is a really redundant process
• It doesn't load the keys from environment variables
• It doesn't try to automatically get the code from the pasted URL unless you use self._token = token.split("?code=")[1].split("&")[0]
• 9 lines of code vs 3 (although it's more "customizable")
• I'm not sure why the old API asks for a username when this one doesn't

Traceback:
spotipy/client/base.py", line 129, in next
return type(result)(**self._get(result.next))
TypeError: init() got an unexpected keyword argument 'tracks'