hackage-server icon indicating copy to clipboard operation
hackage-server copied to clipboard

Solves #1029 - Adds paging to recent packages and recent revisions

Open LeviButcher opened this issue 3 years ago • 6 comments

Implements #1029

Everything Included:

  • [x] Paging support via queryParams (page, pageSize) on recent packages and recent revision (html, rss)
  • [x] Extract HTML and RSS responses from RecentPackagesFeature to HTMLFeature
  • [x] Uses same paging functionality as the browse/search page but without JS (I also updated it to remove Next/Prev if there isn't one)

Possible Issues:

  • I'm not for sure if I handled the cache correctly in the RecentPackagesFeature
  • I have it paging over all the data, I could change it to only page over the last 48 hours worth if desired
  • Some of the support functions I made are not necessarily safe (Usage of head) but I accounted for errors as much as possible

Let me know of any improvements I can make. :+1:

LeviButcher avatar Apr 12 '22 02:04 LeviButcher

@ysangkok I'll of course take a closer look before acting on this, but want to know if from your standpoint you think the issues you raised have been sufficiently addressed?

gbaz avatar Apr 15 '22 15:04 gbaz

The paginator doesn't seem to work the same on the recent packages list, which I think it should. For example, I have one package in my index, and the browse page looks like:

image

Meanwhile, the recent packages listing looks like:

image

I don't think the ellipsis should be there. (I don't really care about the pluralization)

ysangkok avatar Apr 17 '22 16:04 ysangkok

I've fixed the paginator issue @ysangkok described. :+1:

LeviButcher avatar Apr 17 '22 19:04 LeviButcher

A maximum page size like e.g. 200 should be enforced. Otherwise, it is really easy to overload the server. Especially since the parameter is received as a query string parameter, and therefore the overloading could be triggered simply by a link.

ysangkok avatar Apr 19 '22 13:04 ysangkok

Implemented page size restriction. @ysangkok

Do you think the RecentPackages returning all packages is a performance issue? It does use the same caching as before although I wasn't for sure if I refactored it correctly. I can restrict it to only the past 48 worth or some x amount although that will restrict how many pages are possible.

LeviButcher avatar Apr 20 '22 03:04 LeviButcher

I think we should restrict that as well, because it seems malicious if somebody is e.g. requesting page 50 of RecentPackages. If a legitimate user needs to do that, they're probably really trying to achieve something that would be better addressed by a new feature.

ysangkok avatar Apr 23 '22 13:04 ysangkok