rspotify icon indicating copy to clipboard operation
rspotify copied to clipboard

Partial model object with `fields` query parameter

Open ghost opened this issue 5 years ago • 7 comments
trafficstars

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.

ghost avatar Oct 07 '20 10:10 ghost

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

mendelsimon avatar Aug 29 '22 00:08 mendelsimon

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?

kangalio avatar May 23 '23 16:05 kangalio

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.

mendelsimon avatar May 29 '23 18:05 mendelsimon

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?

kangalio avatar May 29 '23 19:05 kangalio

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.

mendelsimon avatar May 29 '23 19:05 mendelsimon

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?

dylif avatar Oct 22 '23 00:10 dylif