Thanks for your heads-up, I will take time to implement this feature after figure out how does it work in Tekore.

from rspotify.

It's actually pretty straigthforward. Whenever a request is made with the credentials, it checks if the token has expired, and in that case, it uses the refresh token to obtain a new access token. The usage is exactly the same as a regular token. This would mean that Credentials in this module would have to become a trait, and be implemented by a regular credentials struct, and by a refreshing credentials struct. That way, they can be used interchangeably, which is achieved with inheritance on Tekore.

from rspotify.

My progress so far

There are a few approaches I've tried to avoid using features for configuring the different kinds of token: regular, cached, refreshing. It'd be better to consider alternatives before adding a new feature for a number of reasons, in my opinion:

• Features can be harder to understand and set up in my experience
• Code filled with #[cfg] statements looks terrible unless it's well managed.
• Features are useful to configure something for an entire library, but sometimes you want to use multiple combinations of them at the same time. Some might want to use a cached token for a part of their program, and a regular one for the rest. I'm not sure how often that happens though.
• There are going to be quite a few features after this release:
  • cli
  • env-file
  • client configuration
  • streams, #166
  • token-cached
  • token-refreshing

The goal is to be able to configure a token as a combination of regular, cached, and refreshing. It shouldn't have any overhead if possible (so it has to happen at compile time), and it should be easy to understand and set up.

Approaches

Generic client over token type

I was initially thinking of using a base TokenHandler trait to be implemented by RegularTokenHandler, CachedTokenHandler or RefreshingTokenHandler, which can be used later by the client for get_token and set_token with whatever code is implemented in them. This makes it easy to specify what type of token you want to use when initializing the client, like so:

let client = ClientCredentialsSpotify::<CachedTokenHandler>::new(creds);

A solution like this not only would make it possible to use different configurations of tokens with a single compiled rspotify library, but also reduce the amount of #[cfg] statements. It would specially be preferred because of the first reason. We'd have to figure out how to use RegularTokenHandler as the default value for usability reasons.

However, not only this isn't exactly what we need, because the user could possibly want a both cached and refreshing token, but also the refreshing one would be quite complicated to implement inside a token handler component without knowing the authentication process that was followed. Not to mention that the initialization of the token handler would have to happen privately inside the client and not outside, because you have to pass it the HTTP client in the initialization, which the user doesn't have access to.

The implementation of refresh_token is inside the client, so RefreshingTokenHandler would require a reference to whichever client is being used, which can quickly turn into a mess. This logic separation doesn't seem to be possible and it falls apart as soon as I try to implement it for multiple reasons (it also requires specialization, here's my failed attempt).

Configuration at runtime

A configuration at runtime is super easy to implement. But it has a few downsides:

• It's an overhead compared to features (not that much to be honest, just a couple if, and it could be optimized away by the compiler, I have to check at https://godbolt.org/)
• It requires either the builder pattern (which I wanted to get rid of with the new architecture) or two separate methods set_token_cached and set_token_refreshing.
• I'm not sure how important it is to be able to be able to configure multiple combinations of token types.

NOTE: Huh turns out rustc can easily optimize the runtime overhead: https://godbolt.org/z/8Mb9K7, so it's less of a problem. Notice how the cached token part isn't included in the binary when using -C opt-level=3.

IDETs

Back when I created a thread on r/rust for opinions for #173, I was sugested the new IDET pattern. It's an interesting take on extensible functionality as an alternative to features, but it seems quite complicated and not exactly what we need. If anyone wants to give it a try go ahead.

All in all, I'm running out of ideas. I'd love more opinions on this, or otherwise we can just use conditional compilation with features -- which isn't that much of a problem anyway imo.

Regarding my attempts, the only question we should consider is: are users really going to need different combinations of token configurations in the same program?

from rspotify.

I am a little bit confused about:

I have to restart my program in order to re-authenticate

Do you mean you have to open a browser and authenticate again ?

from rspotify.

No.

If I let my program run for a while, eventually all requests return 403.
If I restart it, then everything works, without having to open a browser.

from rspotify.

I get your point, I'll do it but I'm not sure when I could implement this feature since I am busy in doing something else :)

from rspotify.

That's understandable!

I was more asking for confirmation that I hadn't missed an option in the API.
I might try to fix it/add the option myself.

from rspotify.

The rspotify library could re-authenticate by calling the function explictly , then it will load the token info from cache, but it could not re-authenticate automatically. Off topic, I think it make sense that the cookie expires when you leave your browser alone for a while :-)

from rspotify.

Just wanted to point out that Tekore already implements this, which is a Python library for the Spotify Web API. This can be helpful to see how the refreshing credentials are used in other libraries, and possibly to see how it's implemented.

from rspotify.

I get your point of Tekore's implementation. My original intution is using a daemon thread to scan the expire_time and refresh it before expired, but it's some kind of clumsy :)

from rspotify.

This is actually not as easy as I thought. Implementing this would mean that whichever method that fetches the token will have to check if it's expired, an in that case, refresh it.

The complicated part comes in when we have to refresh the token: this is a mutable operation, so the method would be mutable as well, and thus propagate up until the endpoints, which would have to be mutable, too.

In the end we'd have a fully mutable Spotify client, which would be much harder to use than an immutable one. But not sure if there's any way to fix this. Perhaps with cells or similars.

from rspotify.

As a summary now that #173 is done:

1. We're going for the "configuration at runtime" option. This is already done.
2. We'll have to use a Cell<Token> instead of Token (possibly Mutex in order to allow concurrency) in order to refresh the token without having to modify the mutability of literally the entire library.

So what's left is just the second point, and adding checks when using the token in order to refresh it.

from rspotify.

Closing in favor of #223

from rspotify.