Skip to content
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

Merged
merged 66 commits into from
Jan 24, 2020

Conversation

hilariocoelho
Copy link
Contributor

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.

@codecov-io
Copy link

codecov-io commented Dec 21, 2019

Codecov Report

Merging #9452 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
modules/notification/indexer/indexer.go 55.07% <100%> (+4.34%) ⬆️
models/repo.go 49.69% <0%> (-0.14%) ⬇️
services/pull/patch.go 67.92% <0%> (+1.25%) ⬆️
modules/git/tree_entry.go 73.33% <0%> (+1.66%) ⬆️
modules/indexer/code/bleve.go 66.07% <0%> (+4.4%) ⬆️
models/repo_indexer.go 70.58% <0%> (+7.84%) ⬆️
modules/indexer/code/git.go 69.14% <0%> (+26.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 465c955...f7a5c9f. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 21, 2019
Copy link
Contributor

@zeripath zeripath left a 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.

models/issue_tracked_time.go Outdated Show resolved Hide resolved
models/org_team.go Outdated Show resolved Hide resolved
models/repo_collaboration.go Outdated Show resolved Hide resolved
options/gitignore/Matlab Outdated Show resolved Hide resolved
modules/git/repo_commit.go Show resolved Hide resolved
@hilariocoelho
Copy link
Contributor Author

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 page field greater than 0. If the request does send that field, then Gitea uses the pagination implemented on this PR.

As a future work the API should be changed in order to add a has_more_pages field to responses when using pagination. The approach I suggest in order to add that has_more_pages field is to get always N+1 requested fields, validates if the result contains N+1, return N and the bool has_more_pages.
It would be something like the following:

// getAPIResponse returns the requested stuff and a "has_more_pages" bool
func getAPIResponse() ([]Stuff, bool) {
	res, err := models.GetStuff(page, pageSize)
	
	apiRes := res[:len(res) -1]
	hasMorePages = len(res) == pageSize+1
	return apiRes, hasMorePages
}

For now I think we can add the currently implemented pagination to Gitea v1 and the has_more_pages implementation goes to Gitea v2.

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.

models/list_options.go Show resolved Hide resolved
models/list_options.go Show resolved Hide resolved
models/repo_watch.go Outdated Show resolved Hide resolved
models/repo_watch_test.go Show resolved Hide resolved
models/repo_watch.go Show resolved Hide resolved
modules/repository/repo.go Show resolved Hide resolved
routers/api/v1/repo/commits.go Outdated Show resolved Hide resolved
routers/api/v1/user/user.go Outdated Show resolved Hide resolved
routers/org/home.go Outdated Show resolved Hide resolved
routers/repo/release.go Show resolved Hide resolved
…OrgID and GetStargazers; fix GetAllCommits headers; reverted some changed behaviors
models/issue_milestone.go Outdated Show resolved Hide resolved
@hilariocoelho hilariocoelho mentioned this pull request Dec 22, 2019
7 tasks
@lunny lunny added the modifies/api This PR adds API routes or modifies them label Dec 22, 2019
@6543

This comment has been minimized.

@6543

This comment has been minimized.

routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
@techknowlogick techknowlogick added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 24, 2020
Copy link
Contributor

@zeripath zeripath left a 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

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 24, 2020
@zeripath
Copy link
Contributor

make lg-tm work

1 similar comment
@zeripath
Copy link
Contributor

make lg-tm work

@zeripath zeripath removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 24, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 24, 2020
@techknowlogick techknowlogick merged commit 1f01f53 into go-gitea:master Jan 24, 2020
@guillep2k
Copy link
Member

Thank you for your hard work!!!

@bjj
Copy link

bjj commented Jun 8, 2020

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

@6543
Copy link
Member

6543 commented Jun 8, 2020

so in short #11800

@mgeerdsen
Copy link

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.
An issue has been opened for this at https://issues.jenkins-ci.org/browse/JENKINS-63048

Just as a suggestion, it would be nice to have these kind of changes (especially API related) in the BREAKING section of the changelog.

@richmahn
Copy link
Contributor

richmahn commented Sep 8, 2020

@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.

@richmahn
Copy link
Contributor

richmahn commented Sep 8, 2020

@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.

@richmahn
Copy link
Contributor

richmahn commented Sep 8, 2020

@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.

@6543
Copy link
Member

6543 commented Sep 8, 2020

@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 :)

@go-gitea go-gitea locked and limited conversation to collaborators Sep 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet