[PR #185] [MERGED] Polish Token and OAuth #297

New issue

Closed

opened 2026-02-27 20:24:09 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 20:24:09 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/185
Author: @ramsayleung
Created: 2/14/2021
Status: ✅ Merged
Merged: 2/17/2021
Merged by: @marioortizmanero

Base: master ← Head: ramsay_optimize_expire_field

📝 Commits (10+)

387bf33 Change expires_at from u64 to DateTime
3bdffd6 Update comment
2f685c7 Change expire_in from u32 to Duration
37f133d Add test to check token.is_expired
34f4423 Update CHANGELOG.md
72e7c12 Change scope from string to hashset
defad50 Use customized module to deserialize/serialize duration
48b5488 Fix cargo clippy error.
1b98a3b Update CHANGELOG
ed9f341 Fix failed exampls

📊 Changes

15 files changed (+309 additions, -125 deletions)

View changed files

📝 CHANGELOG.md (+6 -0)
📝 examples/current_user_recently_played.rs (+5 -4)
📝 examples/oauth_tokens.rs (+8 -9)
📝 examples/ureq/device.rs (+5 -4)
📝 examples/ureq/me.rs (+5 -4)
📝 examples/ureq/search.rs (+9 -8)
📝 examples/ureq/seek_track.rs (+5 -4)
📝 examples/webapp/src/main.rs (+8 -5)
📝 examples/with_refresh_token.rs (+2 -1)
📝 src/lib.rs (+15 -2)
📝 src/model/playlist.rs (+10 -2)
📝 src/oauth2.rs (+171 -47)
➖ src/util.rs (+0 -21)
📝 tests/test_models.rs (+48 -0)
📝 tests/test_with_oauth.rs (+12 -14)

📄 Description

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: https://github.com/serde-rs/serde/issues/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

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/ramsayleung/rspotify/pull/185 **Author:** [@ramsayleung](https://github.com/ramsayleung) **Created:** 2/14/2021 **Status:** ✅ Merged **Merged:** 2/17/2021 **Merged by:** [@marioortizmanero](https://github.com/marioortizmanero) **Base:** `master` ← **Head:** `ramsay_optimize_expire_field` --- ### 📝 Commits (10+) - [`387bf33`](https://github.com/ramsayleung/rspotify/commit/387bf3363770f71af3764c301fc191c628a40679) Change expires_at from u64 to DateTime - [`3bdffd6`](https://github.com/ramsayleung/rspotify/commit/3bdffd64378c32bac38d2ce04e457dc6d740fb35) Update comment - [`2f685c7`](https://github.com/ramsayleung/rspotify/commit/2f685c7de8608b16b96185994be2f4841c450e9e) Change expire_in from u32 to Duration - [`37f133d`](https://github.com/ramsayleung/rspotify/commit/37f133da2d8f74673539eb4eab9481c8fa38b4e7) Add test to check token.is_expired - [`34f4423`](https://github.com/ramsayleung/rspotify/commit/34f44238fee21600d64543cef32bed62cf068e75) Update CHANGELOG.md - [`72e7c12`](https://github.com/ramsayleung/rspotify/commit/72e7c125b5d70459c4f83f15b2e9957403166574) Change scope from string to hashset - [`defad50`](https://github.com/ramsayleung/rspotify/commit/defad5005f14feaa270ee285c7fdb96757b41e0b) Use customized module to deserialize/serialize duration - [`48b5488`](https://github.com/ramsayleung/rspotify/commit/48b54885c6adcdce5b789ce3ad429185c2547e1f) Fix cargo clippy error. - [`1b98a3b`](https://github.com/ramsayleung/rspotify/commit/1b98a3b72c3386d16230c5e52805e2143a0f12e7) Update CHANGELOG - [`ed9f341`](https://github.com/ramsayleung/rspotify/commit/ed9f341dcf0a4c1a40adc3286462ab7ed479565a) Fix failed exampls ### 📊 Changes **15 files changed** (+309 additions, -125 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+6 -0) 📝 `examples/current_user_recently_played.rs` (+5 -4) 📝 `examples/oauth_tokens.rs` (+8 -9) 📝 `examples/ureq/device.rs` (+5 -4) 📝 `examples/ureq/me.rs` (+5 -4) 📝 `examples/ureq/search.rs` (+9 -8) 📝 `examples/ureq/seek_track.rs` (+5 -4) 📝 `examples/webapp/src/main.rs` (+8 -5) 📝 `examples/with_refresh_token.rs` (+2 -1) 📝 `src/lib.rs` (+15 -2) 📝 `src/model/playlist.rs` (+10 -2) 📝 `src/oauth2.rs` (+171 -47) ➖ `src/util.rs` (+0 -21) 📝 `tests/test_models.rs` (+48 -0) 📝 `tests/test_with_oauth.rs` (+12 -14) </details> ### 📄 Description ## 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: ```json { "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: ```rust 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: https://github.com/serde-rs/serde/issues/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. - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [x] This change requires a documentation update ## How Has This Been Tested? - [x] `test_write_token` - [x] `test_token_is_expired` --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>