Polish Token and OAuth

[ramsayleung]

Description

As @Kestrer pointed out in #127, the expires_at and expires_in are not that easy to use:

TokenInfo stores expires_in as a u32, and additionally has the expires_at field. This is not easy to use, instead it should have a single expires field with the type Instant

There is no way to store expires information into a signle field, since the response of requesting to Spotify for an access token looks like this:

{
   "access_token": "NgCXRK...MzYjw",
   "token_type": "Bearer",
   "scope": "user-read-private user-read-email",
   "expires_in": 3600,
   "refresh_token": "NgAagA...Um_SHo"
}

It's possible to deserialize this expires_in to Token.expire field, like this:

    pub fn deserialize<'de, D>(deserializer: D) -> Result<Instant, D::Error>
    where
        D: Deserializer<'de>,
    {
        let elapsed = u32::deserialize(deserializer)?;
        let duration = Duration::from_secs(elapsed as u64);
        let now = Instant::now();
        let instant = now
            .checked_sub(duration)
            .ok_or_else(|| Error::custom("Erreur checked_add"))?;
        Ok(instant)
    }

In order to convert expires_in(elapsed) to Instant, it's necessory to instantiate a Instant object in deserialize function, it turns out to be a stateful function. If we serialize the Token into cache file and deserialize it to Token, then the expires value will change unexpectedly.

Hence, it's necessary to keep two fields(expires_in, expires_at) to handle expire information, the expires_in has been updated from u32 to std::time::Duration, and the expires_at field has been updated from u64 to chrono::DateTime.

For more details about the support of serde for std::time::Instant, check this issue: serde-rs/serde#1375

Motivation and Context

To make rspotify ergonomic to use. Modification:

• Change Token.expires_in from u32 to std::time::Duration
• Change Token.expires_at from i64 to chrono::DateTime<Utc>
• Change Token.scope from String to HashSet.
• Change OAuth.scope from String to HashSet.

Dependencies

None

Type of change

Please delete options that are not relevant.

• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

• test_write_token
• test_token_is_expired

[marioortizmanero]

Great job! I've just included a few commits to have neater iterators for converting between HashSet and Vec, and a few other improvements, although if you consider any of them let me know and revert them.

[ramsayleung]

I've just included a few commits to have neater iterators for converting between HashSet and Vec, and a few other improvements, although if you consider any of them let me know and revert them.

You do a great job to reduce unnecessary operation, it's no way to revert them :)

PS: Perhaps you could take time to review this PR #184 if you are free, it fix some failed CI jobs, then we could merge master branch into current PR later to fix the failed CI.

[ramsayleung]

@marioortizmanero you could take time to review this PR again, I think the required commits are completed :)

[marioortizmanero]

Ok after these comments it should be good to go!

Another thing I would like to consider is whether we'd be better off using BTreeSet instead of HashSet or not. Here's a comparison I've read: https://stackoverflow.com/questions/62558996/what-is-the-time-complexity-of-iterating-btreeset-and-hashset. This is also interesting: https://doc.rust-lang.org/std/collections/index.html#when-should-you-use-which-collection

What we mainly need is:

• Iteration of the elements
• Inserting new elements

It does look like a HashSet is the best way to go, but I just wanted to make sure.

[marioortizmanero]

LGTM! Great work