ClientCredsSpotify allows requests before authenticated and panics with 'Rspotify not authenticated'

[kaleidawave]

Describe the bug

The following example internally panics with 'Rspotify not authenticated' rspotify-0.11.5\src\clients\base.rs:100:14

use rspotify::{clients::BaseClient, ClientCredsSpotify, Credentials};

#[tokio::main]
async fn main() {
    const CLIENT_ID: &str = "...";
    const CLIENT_SECRET: &str = "...";
    let mut client = ClientCredsSpotify::new(Credentials::new(CLIENT_ID, CLIENT_SECRET));

    let results = client
        .playlist_items_manual(
            &"37i9dQZF1DXcBWIGoYBM5M".parse().unwrap(),
            None,
            None,
            Some(5),
            None,
        )
        .await;

    eprintln!("{:#?}", results);
}

Turns out there first needs to be a client.request_token().await.unwrap(); call after creating the client. I feel there should be a intermediate struct so only authenticated clients can call .playlist_items_manual. Then the type system mistake could have caught the mistake I made.

Thanks 🙌

[marioortizmanero]

Yeah, you first need to call client.request_token().await.unwrap();. The thing is that it's hard to do something like this type safe because the authentication eventually expires as well. You could've had the same error by just running that script for something like an hour. If you do have any ideas please let us know, as we would love to avoid issues like this. Perhaps we should improve the error message, at least?

[Spanfile]

Library code should never panic when the user makes a mistake (in this case, not filling in the authentication information or requesting the access token), it should return an error and let the user deal with it however they want. In a similar vein, when the access token eventually expires after an hour and a call returns 401, ideally the library should refresh the token itself and retry the call.

[marioortizmanero]

Yeah, you're right. I'll work on this after we merge some of the PRs we have pending. About the token refreshing, we already implement that, but it's opt-in, so it can still happen because not everyone wants to refresh tokens automatically.

[kaleidawave]

I don't currently completely understand how the Spotify api authentication works but it seems a little complex with the refreshing token thing so I understand why is handled this way! I should have read the docs a little closer the first time. I think that it would be good if the panic output was instead, Rspotify not authenticated, have you run `client.request_token().await.unwrap();` or something. And if this error was handled in the return type for any requests that would be even better.

[marioortizmanero]

We definitely have to improve the error message at least. We could return an error instead of a panic, but that error would be different from the one obtained with expired tokens. The error you were getting here is because no initial token is configured, so we can't even make the request. It would be a variant in ClientError. The expired token is returned by the API, so it's part of HTTPError instead.

I'm hesitant to add a variant to ClientError for this because it's not supposed to happen if the client is configured correctly. When error-handling, everyone will also have to take that variant into account, even though it's not supposed to happen at all. I'm okay with adding it as I understand that panicking is bad, but you could argue that it's not that bad to panic there.

The "proper" fix would be to re-think the architecture a bit so that you can't actually make requests until the first authentication is done (like a builder but fully type-safe). But again, that couldn't possibly handle expired tokens either; authentication is something you should be aware of before using the library. I remember when I did the first redesign this was somewhat complicated:

• You also need an HTTP client instance in that builder structure to do the authentication part. Then, you'll have to either move it to the actual client or similars.
• Even after building the actual client, you need access to the same authentication methods to refresh or request new tokens. You can't just get rid of the first structure. The best fix for this I think would be to move the "builder" itself inside the client after being done. Then, we add a getter to the builder so that the authentication methods can still be accessed. But it's definitely not as easy as that.

I wanted to take a look at newer API libraries to see how they approach stuff like this. But I haven't had time to rethink the architecture again, only for minor and important fixes. Any ideas are appreciated while I do the research.

[github-actions[bot]]

Message to comment on stale issues. If none provided, will not mark issues stale