mirror of
https://github.com/ramsayleung/rspotify.git
synced 2026-04-25 23:45:52 +03:00
[GH-ISSUE #498] Add ID vector size checks to give the user a descriptive error message when calling 'ID batch' methods #164
Labels
No labels
Stale
bug
discussion
enhancement
good first issue
good first issue
help wanted
pull-request
question
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/rspotify#164
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @JonathanBHill on GitHub (Oct 30, 2024).
Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/498
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!
@ramsayleung commented on GitHub (Nov 1, 2024):
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.
@JonathanBHill commented on GitHub (Nov 1, 2024):
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?
@ramsayleung commented on GitHub (Nov 4, 2024):
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