Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IDs v3 #243

Closed
wants to merge 19 commits into from
Closed

Fix IDs v3 #243

wants to merge 19 commits into from

Conversation

marioortizmanero
Copy link
Collaborator

@marioortizmanero marioortizmanero commented Aug 25, 2021

Description

Closes #203

This final approach consists of mixing two of the ideas I explained in #203.

Explanation

It is fundamentally impossible to have an object-safe Id trait because it requires a const field in one way or another that links its type to the variant in Type. So attempting to use dyn for runtime usage (Box<dyn Id> or Vec<Box<dyn Id>>) has to be discarded. Thus, I've created IDs that may hold multiple types instead of a specific one; AnyId serves for any kind of Id, PlayableId can be created with a TrackId or an EpisodeId, etc. Note that these are types and not traits. So when a function wants to take different kinds of IDs, it's not done via generics; it's done via ID conversions.

Say for example you have a tracks: Vec<TrackId> and the function wants a Vec<PlayableId>. In that case, you'd simply pass your tracks with tracks.into_iter().map(|t| t.as_ref()).collect(). Note that we use AsRef for conversions instead of From/Into because it's a cheap conversion; it only requires a pointer cast (that doesn't even use unsafe).

In order to make the previous conversion possible, one has to implement impl AsRef<Id<Playable>> for Id<Track>. The problem here is that after all Id<Playable> and Id<Track> are the same type. It's just the generics that change. So this impl block would conflict with impl AsRef<T> for T, which already exists in the standard library, and makes it impossible.

Given the previous reasoning, we know that each Id must be a different type, discarding Id<T>. The only remaining option here is to have a struct for each Id which implements the Id trait with all of its functionality. Thus, for each kind of ID we need to:

  1. Declare a struct
  2. Implement all required traits for the struct (Id, AsRef...)
  3. Implement all required conversions into group IDs (e.g. TrackId -> PlayableId)

Which is a lot of boilerplate when we have 2+ kinds of IDs. To make it a bit simpler, we can use both macros and blanket implementations. Ideally, we just need:

  • One macro to just declare the struct and implement the Id trait, and same for IdBuf: define_idtypes!
  • One macro to implement the conversions of IDs: define_one_way_conversions!
  • Blanket implementations for the rest of traits (AsRef, Deref...)

The only issue here is that the blanket implementations don't work; you can't just impl<T: Id> AsRef<AnyId> for T because T may be from a foreign crate, just like AsRef. And as we all know that is not allowed by Rust. So in the end we have to make these impl in the first macro instead of with blanket implementations. Which is not really a problem, but large macros are scary.

Even though this explanation is quite complex and may be hard to follow, the idtypes module isn't really that hard to understand. Here I just tried to explain in detail why this is the only way to do it, and how it works. But the user only needs to know that there are different types to represent IDs, even groups of types, and that they can freely convert from single types into groups of types.

Disadvantages

The main disadvantages are:

  • The use of a large macro, although not that complex
  • It's impossible to use type inference to create IDs now. Id::from_uri(...) won't work.
  • Losing the original type of an Id when converting it to something more generic. For example, when converting TrackId into PlayableId, it is impossible to turn it back into TrackId.

But this is literally the only way I've found to fix IDs after lots of different approaches and time invested. Please, if there's anything unclear just ask, and if you find anything with room improvement let me know.

@marioortizmanero marioortizmanero mentioned this pull request Aug 25, 2021
@marioortizmanero
Copy link
Collaborator Author

This is WIP because I still have to fix the tests and docs, but it should work perfectly and is otherwise ready for review. I'll try to fix the docs and tests soon though.

@marioortizmanero
Copy link
Collaborator Author

This PR also makes FullTrack::id a TrackIdBuf instead of Option<TrackIdBuf>; the docs indicate it can't be null: https://developer.spotify.com/documentation/web-api/reference/#object-trackobject

@marioortizmanero marioortizmanero marked this pull request as draft August 25, 2021 16:28
@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Aug 25, 2021

Oh wow. Incredible. start_context_playback completely breaks my solution because it requires the URI of a group type, which is something I assumed didn't happen at all. But when converting to the group type the concrete type is lost, so the URI becomes invalid (it would return spotify:play_context_id:xxxx, which makes no sense in spotify API terms)

I guess I'll keep doing research but I honestly can't believe this has to be so difficult. Maybe I'm asking for too much or for something impossible. Sorry for making such a mess of comments/PRs/etc... If only I found out about things like these before finishing the implementation...

@marioortizmanero
Copy link
Collaborator Author

Closing over #244, I finally got it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Endpoints with lists of Ids don't accept multiple of them
1 participant