jurialmunkey / plugin.video.themoviedb.helper

GNU General Public License v3.0
204 stars 96 forks source link

[BUG] List pagination and limitation parameters are not applied correctly to content paths #779

Closed inb4after closed 1 year ago

inb4after commented 2 years ago

Describe the bug

Continuation of #676

During testing of next page/previous page buttons I've found some issues with TMDbHelper lists' path parameters. I'm using variables/window properties for page= and limit= in the content paths.

The skin code is the same across all list paths, so any differences should be from TMDbHelper. I've also tested the same paths without any variables/window properties involved, using static numbers instead.

  1. Using info=cast, both page= and limit= are not working (reference)
  2. Using info=similar, only limit= is not working (reference)
  3. Using info=discover, only limit= is not working (reference)
  4. There is a strange case with using info=recommendations. It appears page= is working, but limit= only works on the second page, not the first page (reference)

Some paths are working as expected, where the page= parameter does change the list content to the next/previous page and limit= does limit the amount of items per page.

  1. Using info=crew_in_movies, both page= and limit= are working (reference)
  2. Using info=collection, both page= and limit= are working (reference)

I noticed #644 and changes appear to force a limit for info=crew_in_movies and thereby create pagination for the list. That would explain why this path is working, while others are not.

Hopefully, there is a solution that doesn't involve setting a (hard?) limit of 20 for each list, as longer lists with 20+ items can still be manipulated by the skin, in the case of info=crew_in_movies. In any case, these parameters should be available for the skin to use for every TMDbHelper path.

Steps To Reproduce

See above.

Debug log

No response

Screenshots and Additional Info

No response

Checklist

jurialmunkey commented 2 years ago

You cannot change the TMDb item limit. It is hardcoded to 20 results per page in the API https://www.themoviedb.org/talk/50dbfbcf760ee3417809cc3e#:~:text=on%20October%209%2C%202021%20at%2011%3A54%20AM&text=cannot%20be%20changed%2C%20configured%2C%20specified,a%20maximum%20of%20500%20pages.

There are some exceptions, such as cast/crew based lists, where the API provides the full list in once call so that the client can apply sorting and filtering (e.g. filter a movie's crew for crew members that are in sound production; or filter a person's crewed movies for movies where they are the director etc.)

These cast/crew based lists only provide basic details (i.e. name, job, thumb, id), so TMDbHelper does an additional call per movie or tvshow in the list to look-up more details. The longer the list, the more requests required so the limit param is provided in this instance to reduce the total items per page and speed up loading.

It is never possible to increase the number of items provided by the server via the TMDb API (or reduce for that matter either). The limit param can only ever work client side to reduce the number of items by having TMDbHelper manually paginate the full list after it is downloaded from the server.

EDIT: FYI I don't limit cast/crew members as the additional details calls only happen on movies and shows not people. I can add a limit to paginate these manually if it is wanted for consistency reasons but it won't give any real performance advantage.

inb4after commented 2 years ago

Gotcha.

It would be useful if all the lists were manually paginated, so that I can use the page= and limit= parameters with TMDbHelper paths and for consistency across them. I didn't even factor in performance, but it is working well in the skin for the pages that are able to be limited (currently it's set at 10 for these lists, but changes depending on a skin setting).

The only want for my use case is setting a limit smaller than 20. I gave the 20+ item limit example in case other skins or even browsing the addon already relies on unpaginated lists and whether they would start to see pagination if you manually paginated those types of lists.

jurialmunkey commented 2 years ago

I'd only be able to apply a limit param to lists which aren't pre-paginated (e.g. cast). The problem for a pre-paginated list like recommendations is that only factors of 20 will work as a limit - and even then it complicates things code wise.

For example:

Currently the manual pagination approach simply caches the initial full list provided by TMDb and then slices as needed e.g. page=3&limit=7 applies cachedlist[ ((page-1)*limit) : (page*limit) ] which is listitems[14:21]

For a pre-paginated list, this example will cause the slice to travel over 2 API pages. That is, n=15 falls on page=1 of the API but n=21 falls on page=2 when we need both these items in the same request for our manually paginated page=3&limit=7

Obviously we could override the enforced pagination by retrieving 2+ pages at once. However, that would then defeat the purpose of TMDb enforcing the limit (which is to ensure the API remains free to use).

In your use case it would be fine as I know you aren't trying to abuse the limit. However, I don't want to write that code because sadly some selfish users who think it is appropriate to add lists with 10,000+ movies to their library ruin things for everyone.

inb4after commented 2 years ago

Would we be able to make a trade-off where if the limit is not exactly factoring 20 (anything other than 1,2,4,5,10,20), some items will be lost during slicing?

Or, even better, cache the remainder of the sliced items and prepend them to when TMDbHelper reaches the next set of 20? The only downsides then are mildly slow performance at the end of a remainder slice as the new content loads in and really really complicated code šŸ˜…

jurialmunkey commented 2 years ago

Each page is essentially its own instance so there's no guarantee that previous items have been loaded (eg could go directly to page 3 without going past page 2) - so you can't really carry over the items.

I think just dropping the items is not great but one possibility might be to round the limit to a common factor (eg 7 becomes 5; 12 becomes 10 etc). Still not great though.

inb4after commented 2 years ago

What about setting only allowed values on the limit= parameter for the pre-paginated lists only? Most skins are built around widget item limits of 10 and 20, and for my use case I need values 9,19,10,20.

And in the case where the slice is overlapping we can just defer the second page lookup as far away from the first lookup as possible (essentially not looking up 2 pages at once, but more in sequence to avoid the rate limit).

inb4after commented 2 years ago

I just remembered, doesn't TMDbHelper have a rate limit guard?

Hypothetically, if you send too many requests in a short period of time (in our context, switching pages too quickly with a low limit) then the addon will/should suppress the following lookups and leave an empty page.

Would this solve the potential abuse issue and catch it before it reaches the TMDB API?

jurialmunkey commented 2 years ago

No there's no rate limiting. That was removed from TMDbHelper long ago (approx 2 years ago) when it was removed from the API: https://developers.themoviedb.org/3/getting-started/request-rate-limiting Not having client side limiting is a much better experience overall.

TMDb have server safe guards in place to prevent users spamming requests so there's no real risk to TMDb going down. However, they can disable an API key for abuse if it starts causing a lot of traffic. There's 100k daily unique TMDbH users so even just an extra 20 requests per user (i.e. one extra page with details) means an additional 2 million requests.

Like I said before, I don't think your particular usage pattern is a concern - grabbing an extra page here and there isn't that much of a problem. It's more that I write the code to allow it and then some dickhead on Reddit thinks they're really clever for figuring out how to override the default page limit by adding an extra zero.

inb4after commented 2 years ago

The situation you describe shouldn't happen if we set a hidden limit of 20 items if a user has set a custom limit in the path. For example if I set limit=50 or even limit=200, the real limit will still be at limit=20 regardless. I don't really see how that could be abused by increasing it further beyond 20 if we added this safeguard.

As for slicing, continue slicing as 20 items and let the skin handle displaying as many as it wants to (up to the hidden limit of 20).

My final other suggestions (I promise šŸ˜… )

  1. I'm not sure how TMDbHelper handles this but what if we use the user's developer API key instead of TMDbHelper's when making custom requests with limit=. Then there is no danger to TMDbHelper's API key.
  2. Rate limit guard applied only to custom requests with limit=.
jurialmunkey commented 2 years ago

Believe me it already happened for trakt lists. Someone was distrubuting gists on reddit to modify tmdbhelper code and increase page limits to 99 and getting people to install their mod repo with who knows what on it. If I set a hardcode they go and change it.

API keys are per project and shouldn't be individual. My main concern isn't my key getting disabled. my concern is that I know people abuse these things and then we lose free services. Traffic concerns were why trakt stopped serving images and made some other functions VIP only. It's one reason why tvdb requires a subscription now.

inb4after commented 2 years ago

I'll continue working on the skin side, it might involve dropping items, but nothing that wasn't already being done by other skin settings.

inb4after commented 2 years ago

@jurialmunkey Skin workaround for pre-paginated lists is done, just need the info=cast list to be manually paginated so it can use page= and limit=.

inb4after commented 2 years ago

@jurialmunkey Could I bump this feature request to add manual pagination only for the info=cast list?

jurialmunkey commented 1 year ago

@inb4after - I know this has been a ridiculously long time coming but I think I've addressed both of these issues

Cast/Crew lists should now respect limit= param and can be used in combo with page=

Standard TMDb lists with enforced 20 item pagination now accept a length= param which acts as a page multiplier -- e.g. length=3 will retrieve next 3x 20 item pages and join them into one 60 item page.

When using the length multiplier, the next_page item starts the next page and retains the length value -- e.g. If you use length=3 on the first page, then the next page item will be page=4&length=3 and then page=7&length=3 and so on.