ramsayleung / rspotify

Spotify Web API SDK implemented on Rust
MIT License
641 stars 123 forks source link

Partial model object with `fields` query parameter #132

Open ghost opened 4 years ago

ghost commented 4 years ago

Some Spotify endpoints, especially the ones returning a paging object, allow to specify the fields that we want to fetch.

For example, Get a Playlist's Items has a fields query parameter that allow selection of fields, but we can't use it since it would raise an error when trying to deserialize the fields that are not specify :

// Return an Err(ErrorMessage { msg: "convert result failed,  reason: Error(\"missing field `album`\", ... })
spotify.playlist("37i9dQZF1DWXncK9DGeLh7", Some("tracks.items.track.(id,name)"), None);

This behaviour makes the fields parameter useless and forces to fetch data even if we don't need it. Solutions to that would be :

  1. use #[serde(default)] on existing model object fields
  2. use an Option on existing model object fields
  3. create a new object model, for example PartialTrack, that implement either 1. or 2. and make endpoints that use a fields query parameter return such a Partial object instead of a Full or Simplified one.

I think 3. would be the most reasonable solution, moreover it might be possible to implement it using a macro.

mendelsimon commented 2 years ago

I've run into this as well, and I agree with @Sydpy's conclusion.

kangalio commented 1 year ago

I wonder if the additional complexity is even worth it. I feel like that Spotify API feature is mainly useful for dynamically typed languages, where specifying the fields you're working with is a quick low-effort change to reduce load. But in Rust, you'd have to use a different type, and then .unwrap() all the fields you're working with - a significant amount of friction. To keep rspotify's API simple, maybe this feature shouldn't be implemented?

mendelsimon commented 1 year ago

I think there's enough value in this feature for it to not be abandoned. At the time of my previous comment, I had hacked together a proof of concept to test if it actually made a difference for my workflow (fetching the IDs of all tracks in a playlist), and the speed difference was significant.

kangalio commented 1 year ago

a proof of concept to test if it actually made a difference for my workflow (fetching the IDs of all tracks in a playlist), and the speed difference was significant.

That is interesting. Can you share the benchmarking code?

mendelsimon commented 1 year ago

That is interesting. Can you share the benchmarking code?

Unfortunately, it was just a quick hack job to informally test it and was not saved. If I recall correctly, it was pretty much copy-pasting the relevant structs and functions required to make it work, tweaking the struct to only save the ID (I don't recall if I made the other fields Optional or removed them entirely), and passing the fields parameter to only fetch the ID.

dylif commented 1 year ago

I've just ran into this issue. Forgive me if I'm missing something here, but why is the fields parameter provided if using it doesn't work?