Reduce paging inconsistency #34561
Reduce paging inconsistency #34561
Conversation
* disallow bypassing configured limits in api * allow per_page without documenting it * allow limit for the only endpoint only allowing per_page This is an paging inconsistency api review, please comment your opinion about the added code comments * ctx.SetLinkHeader(int(totalNumOfBranches), listOptions.PageSize) missing for several api endpoints should I add them everywhere? * do we want to accept per_page in api? * better compatibility with existing keda github_runner
GiteaBot
added
the
lgtm/need 2
github-actions
bot
added
modifies/api
ChristopherHX
added
the
type/refactoring
| return watches, sess.Find(&watches) | ||
| } | ||
| watches := make([]*IssueWatch, 0, 8) | ||
| listOptions.SetDefaultValues() |
There was a problem hiding this comment.
The new logic will force the pagination but the pagination could be ignored in the old logic.
There was a problem hiding this comment.
the pagination could be ignored in the old logic.
Yes this is exactly one of the inconsistencies, I want to discuss in this PR.
IMO Someone could abuse disabled paging of the rest api controlled by the api client. For reference this function is only used in the Rest Api and TestCode, nowhere else. Where UI code is calling them as well, I only added comments.
Make Gitea do expensive database queries, without constraints.
Other than that SetDefaultValues is not needed here, since SetSessionPagination calls this function.
| listOptions.SetDefaultValues() |
If this is an expected feature, we should add tests that verifies GET /repos/{owner}/{repo}/issues/{index}/subscriptions can bypass paging by specifing the page as 0 or not including it in the query.
There was a problem hiding this comment.
I'm not against most of the changes. Maybe we should mark this as break.
ChristopherHX
added
the
pr/breaking
This is an paging inconsistency api review, please comment your opinion about the added code comments
ctx.SetLinkHeader(int(totalNumOfBranches), listOptions.PageSize) missing for several api endpoints should I add them everywhere?
do we want to accept per_page in api?
GetListOptions replaced almost all custom paging parsing of the rest api
Should callers that expect page=0 to bypass paging be updated to use Listall constant?