[PR #244] [MERGED] Fix IDs v4 #335

New issue

Closed

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

kerem commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/244
Author: @marioortizmanero
Created: 8/30/2021
Status: ✅ Merged
Merged: 9/15/2021
Merged by: @marioortizmanero

Base: master ← Head: fix-ids-4

📝 Commits (10+)

c2046c7 Add UnknownId and some tests
d5d2812 Proper docs thanks to reworked macro
5a52393 Module-level documentation
1ab8ad1 Now with an example!
3b66b9a Stuck fighting compiler error about Deserialize
5d459f4 First draft with newtype
ea435db The compiler hates me
ce0a860 Mixing Attempt #1 and Attempt #2
bbf0cc2 Almost working now
5b1562f Finally fix Offset impl

📊 Changes

26 files changed (+719 additions, -535 deletions)

View changed files

📝 CHANGELOG.md (+6 -5)
📝 examples/client_creds.rs (+3 -3)
📝 examples/with_refresh_token.rs (+7 -7)
📝 rspotify-model/src/album.rs (+6 -13)
📝 rspotify-model/src/artist.rs (+4 -11)
📝 rspotify-model/src/audio.rs (+6 -7)
📝 rspotify-model/src/category.rs (+2 -2)
📝 rspotify-model/src/context.rs (+7 -6)
📝 rspotify-model/src/custom_serde.rs (+1 -1)
📝 rspotify-model/src/idtypes.rs (+447 -217)
📝 rspotify-model/src/lib.rs (+19 -18)
📝 rspotify-model/src/offset.rs (+5 -7)
📝 rspotify-model/src/playing.rs (+1 -2)
📝 rspotify-model/src/playlist.rs (+4 -12)
📝 rspotify-model/src/recommend.rs (+2 -2)
📝 rspotify-model/src/search.rs (+5 -6)
📝 rspotify-model/src/show.rs (+19 -28)
📝 rspotify-model/src/track.rs (+6 -25)
📝 rspotify-model/src/user.rs (+3 -10)
📝 src/clients/base.rs (+4 -0)

...and 6 more files

📄 Description

Description

I've finally understood what the Rust compiler was trying to tell me. This PR implements type-safe ID wrappers, it is the only way I've managed to do it. Everything is thoroughly explained in rspotify-model/src/idtypes.rs, but if you have any questions please do let me know. In summary, I've ended up being able to make Id an object-safe trait, so that it can be used as dyn Id. This way, it is possible to take e.g. Vec<dyn PlayableId>.

This closes #203

Motivation and Context

Now that I've implemented it, the question is: are type-safe IDs worth it? There is currently a partial implementation of IDs in the master branch in which I'm basing this PR. We have to either finish said implementation, or remove it completely in a different PR.

Advantages:

Type safety: with this, it is impossible for the user to make a logic error related to IDs. The user can't mistakenly pass an ID instead of an URI or vice-versa. IDs are grouped up by types now as well; one can't pass the URI of an artist instead of the URI of a track. @Felix-Droop commented in #235 that he did like strongly typed IDs, for example.
Strongly typed IDs also makes endpoints self-documented. Taking a TrackId as a parameter makes it pretty clear what kind of ID is required, in comparison with &str, in which case it would have to be specified in the documentation (or by context).
All IDs have useful methods available for use: Id::uri, Id::url, Id::_type
The model is now more concise. Things like FullTrack only need an id field; uri and _type can be removed.

Disadvantages:

IDs may be more cumbersome to use; there is a considerable difference between endpoint(id) and endpoint(&TrackId::from_uri(id)?).
Library complexity, though IDs are much easier to use than I thought initially; it's just a trait and an implementation for each type. This PR only adds about ~120 lines of code, many of which are documentation and tests.
IDs are Strings instead of str, which require an allocation. If this ends up being important in terms of performance, we could look into crates like smol_str (which may not allocate until the string is longer than X bytes), or even Cow. So performance may not be that much of a problem; we can look into it in the future.
We lose the ability to let the compiler deduce the kind of ID we're dealing with, i.e. Id::from_id won't work anymore. This is only a disadvantage over the current implementation in master, not over not using type-safe IDs at all.

Dependencies

Nothing new

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)

Though it is a breaking change, the only breakage over master occurs when Id::from_id and similars were used directly. Instead, a concrete type has to be provided now, like ArtistId::from_id. The other main breakage is in rspotify's model; some fields have been removed and modified so that they use this new feature instead of just strings. Additionally, IdBuf has been removed, since now it's always owned anyway.

How Has This Been Tested?

There are new tests in rspotify-model/src/idtypes.rs. Take a look in there. The previous tests still pass.

_{🔄 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/244 **Author:** [@marioortizmanero](https://github.com/marioortizmanero) **Created:** 8/30/2021 **Status:** ✅ Merged **Merged:** 9/15/2021 **Merged by:** [@marioortizmanero](https://github.com/marioortizmanero) **Base:** `master` ← **Head:** `fix-ids-4` --- ### 📝 Commits (10+) - [`c2046c7`](https://github.com/ramsayleung/rspotify/commit/c2046c71a0408660ed6b49ea2a2914264865f388) Add UnknownId and some tests - [`d5d2812`](https://github.com/ramsayleung/rspotify/commit/d5d281249c73227870a4b487317ecbe89b4c9993) Proper docs thanks to reworked macro - [`5a52393`](https://github.com/ramsayleung/rspotify/commit/5a52393c384239a5b525505ccf75dd91e8f08806) Module-level documentation - [`1ab8ad1`](https://github.com/ramsayleung/rspotify/commit/1ab8ad1fcb82464a647728b8ed3b4d4b36b45f43) Now with an example! - [`3b66b9a`](https://github.com/ramsayleung/rspotify/commit/3b66b9a7c8a46c2cdb620ae68d7b88f15c3be6b2) Stuck fighting compiler error about Deserialize - [`5d459f4`](https://github.com/ramsayleung/rspotify/commit/5d459f47b543e354d1fdb0911b7654749db94fc6) First draft with newtype - [`ea435db`](https://github.com/ramsayleung/rspotify/commit/ea435db5e9d13bb1c5497842b3b56c9a9ce719d6) The compiler hates me - [`ce0a860`](https://github.com/ramsayleung/rspotify/commit/ce0a860c0e9292d9c752889bc943bee75fc86dea) Mixing Attempt #1 and Attempt #2 - [`bbf0cc2`](https://github.com/ramsayleung/rspotify/commit/bbf0cc22138230749a6ec1d04bea98155891a16e) Almost working now - [`5b1562f`](https://github.com/ramsayleung/rspotify/commit/5b1562f84f59c67e8c8746265e202c816e861b9a) Finally fix Offset impl ### 📊 Changes **26 files changed** (+719 additions, -535 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+6 -5) 📝 `examples/client_creds.rs` (+3 -3) 📝 `examples/with_refresh_token.rs` (+7 -7) 📝 `rspotify-model/src/album.rs` (+6 -13) 📝 `rspotify-model/src/artist.rs` (+4 -11) 📝 `rspotify-model/src/audio.rs` (+6 -7) 📝 `rspotify-model/src/category.rs` (+2 -2) 📝 `rspotify-model/src/context.rs` (+7 -6) 📝 `rspotify-model/src/custom_serde.rs` (+1 -1) 📝 `rspotify-model/src/idtypes.rs` (+447 -217) 📝 `rspotify-model/src/lib.rs` (+19 -18) 📝 `rspotify-model/src/offset.rs` (+5 -7) 📝 `rspotify-model/src/playing.rs` (+1 -2) 📝 `rspotify-model/src/playlist.rs` (+4 -12) 📝 `rspotify-model/src/recommend.rs` (+2 -2) 📝 `rspotify-model/src/search.rs` (+5 -6) 📝 `rspotify-model/src/show.rs` (+19 -28) 📝 `rspotify-model/src/track.rs` (+6 -25) 📝 `rspotify-model/src/user.rs` (+3 -10) 📝 `src/clients/base.rs` (+4 -0) _...and 6 more files_ </details> ### 📄 Description ## Description I've finally understood what the Rust compiler was trying to tell me. This PR implements type-safe ID wrappers, it is the only way I've managed to do it. Everything is thoroughly explained in `rspotify-model/src/idtypes.rs`, but if you have any questions please do let me know. In summary, I've ended up being able to make `Id` an object-safe trait, so that it can be used as `dyn Id`. This way, it is possible to take e.g. `Vec<dyn PlayableId>`. This closes #203 ## Motivation and Context Now that I've implemented it, the question is: are type-safe IDs worth it? There is currently a partial implementation of IDs in the `master` branch in which I'm basing this PR. We have to either finish said implementation, or remove it completely in a different PR. Advantages: * Type safety: with this, it is impossible for the user to make a logic error related to IDs. The user can't mistakenly pass an ID instead of an URI or vice-versa. IDs are grouped up by types now as well; one can't pass the URI of an artist instead of the URI of a track. @Felix-Droop commented in #235 that he did like strongly typed IDs, for example. * Strongly typed IDs also makes endpoints self-documented. Taking a `TrackId` as a parameter makes it pretty clear what kind of ID is required, in comparison with `&str`, in which case it would have to be specified in the documentation (or by context). * All IDs have useful methods available for use: `Id::uri`, `Id::url`, `Id::_type` * The model is now more concise. Things like `FullTrack` only need an `id` field; `uri` and `_type` can be removed. Disadvantages: * IDs may be more cumbersome to use; there is a considerable difference between `endpoint(id)` and `endpoint(&TrackId::from_uri(id)?)`. * Library complexity, though IDs are much easier to use than I thought initially; it's just a trait and an implementation for each type. This PR only adds about ~120 lines of code, many of which are documentation and tests. * IDs are `String`s instead of `str`, which require an allocation. If this ends up being important in terms of performance, we could look into crates like `smol_str` (which may not allocate until the string is longer than X bytes), or even `Cow`. So performance may not be that much of a problem; we can look into it in the future. * We lose the ability to let the compiler deduce the kind of ID we're dealing with, i.e. `Id::from_id` won't work anymore. This is only a disadvantage over the current implementation in `master`, not over not using type-safe IDs at all. ## Dependencies Nothing new ## 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) Though it is a breaking change, the only breakage over master occurs when `Id::from_id` and similars were used directly. Instead, a concrete type has to be provided now, like `ArtistId::from_id`. The other main breakage is in rspotify's model; some fields have been removed and modified so that they use this new feature instead of just strings. Additionally, `IdBuf` has been removed, since now it's always owned anyway. ## How Has This Been Tested? There are new tests in `rspotify-model/src/idtypes.rs`. Take a look in there. The previous tests still pass. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>