Initial id type proposal

[kstep]

Description

This is a refactoring/change proposal. Introduces several changes and refactorings to the API:

• new Id type to represent Spotify id/URI,
• a new ClientError variant: InvalidId(IdError),
• removed .get_id() and .get_uri() methods,
• introduced new dependency: itertools, used it in API methods,
• added AsRefStr for Type.

The main point of the change is to introduce Id type. This type abstracts out the following Spotify id/URI operations:

• Spotify id/URI parsing and validation,
• Spotify URI formatting,
• zero-copy: keeps just a slice of original data,
• throws an IdError error if id/URI is invalid.

This type replaces both .get_id() and .get_uri() methods.

Also, Spotify methods are changed to use this type instead of .get_id() and .get_uri().
This type, being zero-copy, should also reduce the number of allocations.

This change is a proposal only!

Open Questions/TODO

• I'm not sure where should I put the Id and IdError types, put them into model/mod.rs for now.
• I'm not sure about tests for Id placement, so I left them in model/mod.rs for now.
• Maybe more test-cases should be added.
• Maybe it's better to use Cow<'_, str> and Into<Cow<'_, str>> types for internal Id data representation and method arguments.

Motivation and Context

As per TODO comments in code, and as a part of the refactoring effort as per #127 and #160, I decided to propose a new type to abstract out Spotify id/URI operations. Overall improvements:

• better input data validation (ids, URIs): invalid ids and URIs now produce errors,
• better type-safe Spotify ids representation,
• fewer allocations during Spotify ids parsing and processing.

Type of change

• 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?

• I updated tests related to .get_id()/.get_uri() methods.
• I ran all the enabled tests using default configuration (with my own credentials in .env) with cargo test command.

---

This change is 

[kstep]

Is it a good idea to move Id parsing/validation to our end users? If we accept Id everywhere, user's will have to do some work to parse ids and create Id instances. It makes our library less user-friendly. Are you sure it's OK?

[marioortizmanero]

Is it a good idea to move Id parsing/validation to our end users? If we accept Id everywhere, user's will have to do some work to parse ids and create Id instances. It makes our library less user-friendly. Are you sure it's OK?

IMO passing Id to the endpoints instead of &str is beneficial because the user can choose how to parse it when they know whether it's an ID or a URI. It's also a bit more type-safe.

I would love to hear @ramsayleung's opinion about this.

[marioortizmanero]

I can see that you're really avoiding allocations to progress towards #160. I wanted to disscuss about this before we make any changes because it's a somewhat complicated topic in Rust. I've been trying to learn about when to use &str and when not to and I've come across a few sources:

• https://www.youtube.com/watch?v=gpyl_nEomDQ this video measures the performance differences between using String and &str. The end result is that by using &str you can drop from 85ns to 35ns, but that's when you're only benchmarking the strings, and not an entire library, and the difference is just 50ns after all.
• Lifetimes of App, Arg, ArgGroup: removing or relaxing clap-rs/clap#1041 discusses this exact issue in the well-known clap-rs library. Unfortunately, there's not much discussion about it yet, but it's a good idea to learn more about what other libraries do, and their approaches.

I personally think that these kind of optimizations can be a pain for both programmers and users, and most times they aren't even noticeable. Specially if we end up propagating the use of &str or Cow<&str> to the model structs. I would like to see a benchmark for this library comparing these changes (not necessarily by you). There are definitely other more important parts of the library we can focus on to make it more performant. I for one would like to run https://github.com/jonhoo/inferno with Rspotify, but I haven't had the chance yet. Then, we can discuss whether any performance-focused changes are worth it or not. As to your Cow<&str> recommendation, I would like to consider that as well, but not having seen it in any big libraries I would think it's not a good general-purpose choice. It's also mentioned in the clap-rs issue, but they didn't exactly succeed with it.

As far as I know, this library never intended to be a performant API wrapper for Spotify. In fact, we don't say anything about what the goals of Rspotify are in the README. Being the most popular wrapper in Rust for the Spotify API, my assumption would be that Rspotify aims to be the most flexible and feature-full one, which isn't fully compatible with being the most performant one.

[kstep]

I see your point here. Actually, I gave Cow<&str> a try before, but the end result was very complex, so for the first version I ended up with a wrapper around &str. Anyway, if it's not too difficult to have fewer allocations, why not do it anyway? As far as I can see (although my point of view is a little biased, as I'm the author of the code), the Id type is simple enough to work with, and being allocation free is just a nice bonus.

[kstep]

I resolved the conflicts, cleaned and reformatted the code a little bit. Please feel free to check if it's OK now.

[marioortizmanero]

Beside the small nitpicks I commented, LGTM! What do you think, @ramsayleung?

[ramsayleung]

Beside the small nitpicks I commented, LGTM! What do you think @ramsayleung

Yep, it's same to me, beside the small nitpicks, it looks good to me now :)

[kstep]

Tests started to fail on the authentication stage. I'm pretty sure, it's unrelated to my changes. It's either some transient error, or some auth token got invalid for some reason, or the API got changed in some way.

[marioortizmanero]

I'm merging the PR then! Thanks a bunch for the changes and for your patience, @kstep, it's been a long time haha. If you want to change anything just make a new PR as this one is huge already.

As to the Auth issue, I'll take a look later then.

[ramsayleung]

Cheers!