-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
API add/generalize pagination #9452
API add/generalize pagination #9452
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9452 +/- ##
==========================================
+ Coverage 42.18% 42.24% +0.05%
==========================================
Files 610 610
Lines 80274 80274
==========================================
+ Hits 33865 33909 +44
+ Misses 42249 42202 -47
- Partials 4160 4163 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. This is a huge PR!
Some comments:
- If you are passed in an engine you cannot use x. You MUST use the engine. The engine is often a session in a transaction and you must do the queries within that transaction. This is why my comment is request changes.
- Paging will often require counts to precalculate the number of pages or else at least some way of telling if there is another page - requesting one more entity may be a way of doing that if it's not too expensive. The UI needs to be considered.
- AFAICS this adds mandatory paging to large parts of the API - this likely constitutes a breaking change. We can make this non breaking by making paging non mandatory but that likely obviates some of the benefits. I'm not sure and it needs discussion.
In future if we do want to provide all results we should stream the results to the ctx.Writer likely with some internal batching of requests.
…d; fixed gitignore
Like spoken on Discord channel, this PR doesn't make any breaking change. Although this is a quick implementation of pagination to Gitea's API. API will return all results to requests that are sent without a As a future work the API should be changed in order to add a
For now I think we can add the currently implemented pagination to Gitea v1 and the It is also important to synchronize these changes with gitea/go-sdk#203. Will work on that as soon as this is ready to be merged. |
…OrgID and GetStargazers; fix GetAllCommits headers; reverted some changed behaviors
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add page to /api/v1/users/search
…into list_pagination
make lg-tm work |
1 similar comment
make lg-tm work |
Thank you for your hard work!!! |
This breaks drone.io integration. When it fetches /api/user/repos it triggers pagination and only gets the first 10 results. tl;dr seems to be that it sends no pagination options at all, but gets some kind of default. The drone.io loop for fetching repo lists is here: https://github.com/drone/drone/blob/master/service/repo/repo.go which appears to try to set a size 100. The driver is in another source base here: https://github.com/drone/go-scm/blob/master/scm/driver/gitea/repo.go . It ignores the options (including size and pagination) and there might also be issues with https://github.com/drone/go-scm/blob/master/scm/client.go at the bottom where it parses out pagination information to pass back to the outer loop. This has also been discussed here: https://discourse.drone.io/t/drone-only-shows-10-repositories-from-gitea/6922 |
so in short #11800 |
Sorry to add to this merged PR, but I just wanted to point out that this also leads to problems when using the gitea plugin in Jenkins. This can potentially lead to disabled jobs in Jenkins which then could potentially be discarded as old items according to configuration. Just as a suggestion, it would be nice to have these kind of changes (especially API related) in the BREAKING section of the changelog. |
@spawn2kill How is this not a breaking change? We didn't use to page these results, now with the new version, our apps couldn't find the record they wanted because they were only getting 30, rather than all. Wouldn't that be a breaking change if you now have to use page=# ? We updated to this version and had a bunch of problems due to this. |
@zeripath You were right in this being a breaking change. If an app expects it to not be paged, but now you have to loop through page numbers until no results, that can be an issue. |
@spawn2kill I now see your comment about if you don't use page=# in your URL it will return all. But we don't use page=#, and it only returned 30, the number set in the app.ini file. |
@richmahn pleace open a new issue specific for that kind of api you have issues with - of course you can link to this pull To All: DONT comment on closed pulls, open new issues an reference :) |
This PR adds pagination to List requests. It should be merged along with gitea/go-sdk#203.
It is backwards compatible, no breaking changes found. Unit tests and integration tests are all successful without making any significant change.