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

Fixed count of filtered issues when api request. #12275

Merged

Conversation

hinoshiba
Copy link
Contributor

The search results for "The issues" will respond with the total number of issues in repository instead of the number of search results.
This result in pages that do not have a value in rel = last on the response HTTP header "Link".

  • The target API
    • api/v1/repos/issues/search
    • api/v1/repos/{repo}/issues/

If you create about 10 "The issues" and close 2 of them, the final link is page=10 , even if ?limit=1&page=1&state=open. In such cases, the final link should be page=8. This has been fixed.

And, there was an extra reservation for the slice the related code, so I fixed it.

@hinoshiba hinoshiba changed the title Fixed count of filtered issues when api request. [WIP] Fixed count of filtered issues when api request. Jul 19, 2020
@hinoshiba hinoshiba changed the title [WIP] Fixed count of filtered issues when api request. Fixed count of filtered issues when api request. Jul 19, 2020
@lafriks lafriks added modifies/api This PR adds API routes or modifies them type/bug labels Jul 19, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jul 19, 2020
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 19, 2020
@hinoshiba
Copy link
Contributor Author

hinoshiba commented Jul 19, 2020

https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR160-R171 and https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR179-R191 are same

https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR336-R342 and https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR350-R360 are quite similar

can you dedublicate it by store the Options Struct into a variable you can pass to both functions?

Is possible. However, I don't think it makes much sense in terms of processing.
They are said to be the same, but they all differ in the content of "ListOptions", mainly "PageSize".

The "models.CountIssues" and "model.Issues" arguments are references.
I think either of these needs to be done before the reference can be used safely.

  1. Not run in parallel.
  2. Copy later.

I think it will probably run in parallel, so I didn't think it made much sense in terms of processing.

I think I can reuse the structure by splitting the option structure and using two arguments. However, because it reduces memory efficiency, I made redundant entries.

@6543
Copy link
Member

6543 commented Jul 19, 2020

🤔

@6543
Copy link
Member

6543 commented Jul 19, 2020

Will test it tomorow :)

@hinoshiba
Copy link
Contributor Author

https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR160-R171 and https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR179-R191 are same
https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR336-R342 and https://github.com/go-gitea/gitea/pull/12275/files#diff-6a82b880ff1ae1c2e9b577046e45a6cbR350-R360 are quite similar
can you dedublicate it by store the Options Struct into a variable you can pass to both functions?

Is possible. However, I don't think it makes much sense in terms of processing.
They are said to be the same, but they all differ in the content of "ListOptions", mainly "PageSize".

The "models.CountIssues" and "model.Issues" arguments are references.
I think either of these needs to be done before the reference can be used safely.

  1. Not run in parallel.
  2. Copy later.

I think it will probably run in parallel, so I didn't think it made much sense in terms of processing.

I think I can reuse the structure by splitting the option structure and using two arguments. However, because it reduces memory efficiency, I made redundant entries.

I missed.
I didn't actually look back and forth. There is no parallel processing. ;-D
Sorry,

Will update it after confirming the test.

Co-authored-by: 6543 <6543@obermui.de>
@hinoshiba hinoshiba changed the title Fixed count of filtered issues when api request. [WIP]Fixed count of filtered issues when api request. Jul 20, 2020
filteredCount, err = models.CountIssues(&models.IssuesOptions{
ListOptions: models.ListOptions{
Page: ctx.QueryInt("page"),
PageSize: issueCount,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PageSize: issueCount,
PageSize: setting.UI.IssuePagingNum,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ if you dedub things this is not needed anyway ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the purpose of getting the maximum value that matches the filter.
Therefore, the current maximum value is set as an option.

@6543
Copy link
Member

6543 commented Jul 20, 2020

I would say dedub/refactor things and I'll check it again - for me it looks like there is something still missing but I'll dig deeper into it after you changed the code :)

@6543
Copy link
Member

6543 commented Jul 20, 2020

@hinoshiba let me know if you are ready for review again :)

@hinoshiba
Copy link
Contributor Author

@hinoshiba let me know if you are ready for review again :)

Ok, Just a moment.

@hinoshiba hinoshiba changed the title [WIP]Fixed count of filtered issues when api request. Fixed count of filtered issues when api request. Jul 20, 2020
@hinoshiba
Copy link
Contributor Author

@6543 Finished update.

@6543
Copy link
Member

6543 commented Jul 22, 2020

just format nits: hinoshiba#1

@6543 6543 mentioned this pull request Jul 22, 2020
@6543
Copy link
Member

6543 commented Jul 23, 2020

EDIT: thanks

@6543
Copy link
Member

6543 commented Jul 23, 2020

make fmt is posible needed at this point ...

@hinoshiba
Copy link
Contributor Author

make fmt is posible needed at this point ...

Ok, I'm Update.

@6543
Copy link
Member

6543 commented Jul 23, 2020

well github code suggestions have there up and downs ;)

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 23, 2020
@6543
Copy link
Member

6543 commented Sep 1, 2020

can you resolve the conflict?

It looks like nobody had time to look at it :/

@hinoshiba
Copy link
Contributor Author

Ok, I just updated.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2020

Codecov Report

Merging #12275 into master will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12275      +/-   ##
==========================================
- Coverage   43.09%   43.08%   -0.01%     
==========================================
  Files         658      658              
  Lines       72455    72477      +22     
==========================================
+ Hits        31226    31230       +4     
- Misses      36175    36183       +8     
- Partials     5054     5064      +10     
Impacted Files Coverage Δ
routers/api/v1/repo/release.go 33.60% <33.33%> (+0.54%) ⬆️
routers/api/v1/repo/issue.go 50.79% <40.00%> (-2.47%) ⬇️
models/issue.go 56.64% <73.33%> (+0.18%) ⬆️
modules/indexer/stats/db.go 43.47% <0.00%> (-17.40%) ⬇️
modules/indexer/stats/queue.go 64.70% <0.00%> (-11.77%) ⬇️
modules/charset/charset.go 68.53% <0.00%> (-4.50%) ⬇️
modules/git/utils.go 73.77% <0.00%> (-3.28%) ⬇️
modules/process/manager.go 72.50% <0.00%> (-2.50%) ⬇️
modules/git/repo.go 46.19% <0.00%> (-0.51%) ⬇️
services/mirror/mirror.go 19.93% <0.00%> (+1.35%) ⬆️
... and 2 more

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 b510927...630f842. Read the comment docs.

models/issue.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot 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 Sep 24, 2020
@techknowlogick techknowlogick merged commit 6fa19a8 into go-gitea:master Sep 24, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 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. modifies/api This PR adds API routes or modifies them type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants