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

Add ID vector size checks to give the user a descriptive error message when calling 'ID batch' methods #498

Open
JonathanBHill opened this issue Oct 30, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@JonathanBHill
Copy link

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 statements
    • assert!(id_vector <= limit, "ID vector is too large ({}). Your vector of <specific ID type> must be equal to or smaller than {}.", input, limit);
  • Enum
    • must have variants representing each batch method in the BaseClient trait
    • method for checking vector length against the variant's hardcoded maximum
    • method for formatting an error or panic string with the specific length information that triggered the error

Describe alternatives you've considered

  • The easiest alternative would be an addition to the documentation that might look something like the following based on the BaseClient.artists() method documentation
    • /// 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!

@JonathanBHill JonathanBHill added the enhancement New feature or request label Oct 30, 2024
@ramsayleung
Copy link
Owner

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:

  1. backward compatibility: if we introduce a maximum limit for the batch API, either in build time or runtime, all existing applications and libraries which depends on this library with items exceeding the limit will all break, it's not good to have this problem
  2. the maxmium value is determined by the Spotify server, which is out of our control, it could be static value, or it could be dynamic value, how could you find out the correct maxmium value and ensure keeping sync of the latest value?

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

how could you find out the correct maximum value and ensure keeping sync of the latest value?

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.

backward compatibility: if we introduce a maximum limit for the batch API, either in build time or runtime, all existing applications and libraries which depends on this library with items exceeding the limit will all break, it's not good to have this problem

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.

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.

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

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.

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.

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.

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
2 participants