search

nukeop / nuclear

Streaming music player that finds free music for you
https://nuclearplayer.com
GNU Affero General Public License v3.0
12.14k stars 1.06k forks source link

Import Last.fm favorites gives "Error: Invalid number of favorites [2664]" #881

Closed harleypig closed 2 years ago

harleypig commented 3 years ago

Just what the title says ... I logged in to last.fm with no apparent issues. Nuclear appears to be scrobbling correctly.

Clicking the Import button for Last.fm Favorites gives: "Error: Invalid number of favorites [2664]"

nukeop commented 3 years ago

Thanks for reporting, we'll look into it.

nukeop commented 3 years ago

Do you have the same nickname on last.fm as here? Maybe it's because of a large number of favorites and the API can't handle it?

nukeop commented 3 years ago

https://www.last.fm/api/show/user.getLovedTracks

According to the docs the "limit" param controls how many tracks we download. As it doesn't indicate the maximum limit, we try to download all the tracks at once. Apparently 2664 is too many.

nukeop commented 3 years ago

Looks like the max is 1000

json-lou commented 3 years ago

Hi, I would be interested in working on this as my first issue if still available! :)

nukeop commented 3 years ago

Sure, I just identified the problem yesterday. Let me give you some more info for context:

https://github.com/nukeop/nuclear/blob/d0b384789bd1eb3b8d5a136a4aa1dd46c4db3e97/packages/core/src/rest/Lastfm.ts#L161

This is where we download the favorites. Currently, this function returns a promise with the response, but now we need to check if the limit is greater than 1000, and paginate our requests as necessary to download the entire collection of favorite tracks.

Please remember to cover this functionality with tests as well.

nukeop commented 3 years ago

Ok looks like this limit was actually mentioned, and it's partially implemented in the related action: https://github.com/nukeop/nuclear/blob/d0b384789bd1eb3b8d5a136a4aa1dd46c4db3e97/packages/app/app/actions/importfavs.js#L49

It partially implements pagination for n>1000.

json-lou commented 3 years ago

@nukeop I have finished implementing this feature, I am just finishing up the tests now. What do you think would be the best way to test this feature?

Since this API call requires a userId, should I just use my own Last.fm account for the tests? For ex, I have 1000+ songs liked on my account so I could just test making multiple calls (with different pages passed in) to this endpoint and verify the responses are valid. I could add this to lastfm.test.ts. Would this be sufficient?

Let me know if you have any suggestions, thanks!

KiritoStorm commented 3 years ago

Is it possible to fix this soon? When I see this right, it just needs to be tested.