-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add ID vector size checks to give the user a descriptive error message when calling 'ID batch' methods #498
Comments
Thanks for your callout :) As for the first approach, I believe the idea is similar, to set up a maxium limit for the batch API, either in compile time or in runtime, but I don't incline to this approach because of the several considerartions:
Therefore, I also don't incline to update the doc to describe the behavior unless you know exactly what's the maxmium value I believe it's doable to add a section into the FAQ or wiki to demonstrate this weird behavior of batch API, I would greatly appreciate it if you could add this section. |
I think this is the most important question to answer first. I obtained the current API maximums from Spotify's API docs for each API endpoint where a maximum is explicitly defined (see the IDs field description under the GET request section). These values are static by nature of being included in their documentation to the best of my understanding but out of an abundance of caution, I have reached out to Spotify to clarify whether or not those are likely to ever change and if there is an endpoint fore the sole purpose of requesting what some of these values are in real time.
This request is not intended to introduce an artificial limit. It is just to enforce what Spotify has already defined as the maximum. Assuming Spotify does not change the maximums, then all this would do is check to make sure the number of IDs is within what Spotify will allow. If the number of IDs exceeds what Spotify will allow, then instead of a generic 400 error code, developers would be given a message that clearly points to the size of the ID vector as being a problem that is preventing the request from being completed. After delving into the source code a little more, I found that methods that call the paginate functions already implement a default maximum via the rspotify::Config.DEFAULT_PAGINATION_CHUNKS constant (which is set to 50). With this in mind, I think a solution that automates the process for users could be pretty simple. Since the current maximums that Spotify uses are known for each endpoint, a silent check on the ID vector size could be hard coded using additional Config constants such as SMALL_PAGINATION_CHUNKS = 20 & LARGE_PAGINATION_CHUNKS = 100 for the two other maximums that Spotify has defined in addition to the "default" 50. This check would have to be done before the ID vector is converted into the ids string and raise an error before the api request is made in order to preserve existing code and backwards compatability.
Forgive me for needing to clarify, but where are the FAQ and wiki pages? Are you just referring to the README file or is there another location? |
Thanks for your clarification, I am unware that the current API maximums from Spotify's API docs for each API endpoint where a maximum is explicitly defined.
Now we have solved the problem about how to find out the maxium value, as long as you get the feedback from Spotify, we probably could solve the second problem about how to keep sync of the latest value.
I like this proposal, it not only could help us solve this problem, but also try to be as seamless as possible for user, thanks for this idea. The wiki is here: https://github.com/ramsayleung/rspotify/wiki |
Is your feature request related to a problem? Please describe.
The problem itself is that several methods that request 'batch' data such as albums, tracks, artists, playlists, tracks_features, etc. all panic if they are given more items (as the respective ID type) than the Spotify API allows. The Http error is not very descriptive in pointing a new user towards the cause. The size limit also varies from method to method so it ends up breaking up my flow when I have to check the docs to see what the maximum is. This is really more of a quality of life request than a full blown feature request. That said, I do want to propose some solutions that might help curb the issue and minimize the need for users to go to the API docs.
Describe the solution you'd like
The best solution would be one that notifies the user that their code will not run successfully with too many IDs. These are some of the ones I've come up with so far:
assert!(id_vector <= limit, "ID vector is too large ({}). Your vector of <specific ID type> must be equal to or smaller than {}.", input, limit);
Describe alternatives you've considered
/// Returns a list of up to 20 artists given the artist IDs, URIs, or URLs.
///
/// Parameters:
/// - artist_ids - a list of artist IDs, URIs or URLs
///
/// [Reference](https://developer.spotify.com/documentation/web-api/reference/#/operations/get-multiple-artists)
Additional context
I would be happy to make a pull request for it myself. I just wanted to post a request first to get input before throwing something together that isn't in line with the project's long term vision/approach. Thanks for the package! It's been a joy to work with so far!
The text was updated successfully, but these errors were encountered: