[Auth restructure 3] Split up Spotify into multiple clients
#215
Conversation
marioortizmanero
force-pushed
the
auth-rewrite-part2
branch
from
b34be92
to
8397c9b
Compare
| payload.insert(headers::SCOPE, &scope); | ||
| payload.insert(headers::STATE, &oauth.state); | ||
| // payload.insert(headers::CODE_CHALLENGE, todo!()); | ||
| // payload.insert(headers::CODE_CHALLENGE_METHOD, "S256"); |
There was a problem hiding this comment.
According to the Spotify document, code_challenge_method and code_challenge are required when constructing the authorization URI. Why would you like to comment this code? It's still WIP?
There was a problem hiding this comment.
I've left the PKCE implementation for a future PR
|
One thing I'm still not sure about, though: Maybe |
The link is broken for the part-4 PR has been merged. |
|
Also you should wait to review all the PRs before merging any because otherwise the diff is going to have more unnecessary stuff (like what just happened after merging the fourth PR). If you didn't know you can always revert the |
Yep, it makes sense. You could to merge master into this PR since it includes some changes have been merged into master by |
|
Sorry I don't quite understand your last comment |
|
Sorry for the ambiguity, I mean, you could merge master into this |
|
You mean merge this into master? Well, it'd merge into auth-rewrite-part2 and we'd have the same problem in that case. Or have you reviewed it already? |
Alright, I could just manually ignore the diff which has been merged into master in PR
Not yet, I just took a rough review before, to check what has been modified. Then I will review this PR with caution, since it's a huge PR, it will take some time. |
What I was saying before is that you can revert the auth-rewrite-part4 PR and apply it again after you're done:
But if you're ok with manually doing it then no problem. |
| /// `RSPOTIFY_CLIENT_ID` and `RSPOTIFY_CLIENT_SECRET`. You can optionally | ||
| /// activate the `env-file` feature in order to read these variables from | ||
| /// a `.env` file. | ||
| pub fn from_env() -> Option<Self> { |
There was a problem hiding this comment.
Perhaps we could return ClientResult for from_env() functions instead of Option<Self>?
There was a problem hiding this comment.
The thing is that env::var returns just an Option, so there's nothing else to report with a Result in that function.
I also wanted to do this for Token::from_cache, but then I would have to re-think this in src/client_creds.rs:
/// Tries to read the cache file's token, which may not exist.
///
/// Similarly to [`Self::write_token_cache`], this will already check if the
/// cached token is enabled and return `None` in case it isn't.
#[maybe_async]
pub async fn read_token_cache(&self) -> Option<Token> {
if !self.get_config().token_cached {
return None;
}
let token = Token::from_cache(&self.get_config().cache_path)?;
if token.is_expired() {
// Invalid token, since it doesn't have at least the currently
// required scopes or it's expired.
// TODO: update with error?
None
} else {
Some(token)
}
}In that case we now have to return an error when the scopes don't match instead of (Some/None), and I'm not sure which one to pick. Perhaps we should add a new variant?
There was a problem hiding this comment.
Well, I think Some/None is more reasonable, since we should return None when we could not find matched data, which is following the semantics of Option type.
|
Also, remember to only accept (and not merge) this PR after you're done. Then you can review the second PR, do the same, and after you agree with all of them merge them. That way the diff can be easy to follow. |
|
Yeah, everything looks good to me, approve :) |
Part 3 of splitting up #207, possibly the biggest of all changes. There's no way to reduce the number of changes here, really, as it requires moving most of the client's code into multiple files and different structures. But it doesn't really introduce that many changes. The endpoints themselves aren't changed at all and can be ignored, as they just were moved into traits.
The main library should be able to compile without problems after this PR.