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

Partial model object with fields query parameter #132

Open
ghost opened this issue Oct 7, 2020 · 6 comments
Open

Partial model object with fields query parameter #132

ghost opened this issue Oct 7, 2020 · 6 comments

Comments

@ghost
Copy link

ghost commented Oct 7, 2020

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 ghost changed the title Model fields Partial model object with fields query parameter Oct 7, 2020
@mendelsimon
Copy link

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

@kangalio
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

dylif commented Oct 22, 2023

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?

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

No branches or pull requests

3 participants