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

Fix the duplicated repositories load on dashboard #29878

Closed
wants to merge 3 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 18, 2024

Even if there is count_only, the previous code still invoked SearchRepository with 50 page size. That's an unnecessary waste. This PR will invoke CountRepository if there is count_only parameter.

@lunny lunny added performance/speed performance issues with slow downs backport/v1.21 This PR should be backported to Gitea 1.21 labels Mar 18, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 18, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 18, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 18, 2024
@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 Mar 18, 2024
Comment on lines 629 to 632
ctx.JSON(http.StatusInternalServerError, api.SearchError{
OK: false,
Error: err.Error(),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know from what time, the code started exposing internal error messages to end users. There were some worries about "the internal error message may contain sensitive information for server environment".

And, I don't see why it should use api.SearchError here, is the response really handled by frontend JS?

Copy link
Member Author

@lunny lunny Mar 18, 2024

Choose a reason for hiding this comment

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

Looks like it's a very old code. I sent a new commit 413b6c0 to return a simple error for non-admin users.

Copy link
Contributor

Choose a reason for hiding this comment

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

And, I don't see why it should use api.SearchError here, is the response really handled by frontend JS?

And, I don't see why it should use api.SearchError here, is the response really handled by frontend JS?

Looks like it's a very old code. I sent a new commit 413b6c0 to return a simple error for non-admin users.

It doesn't look good to me if it would fill the code base with a lot of if ctx.Doer != nil && ctx.Doer.IsAdmin {

Copy link
Member Author

Choose a reason for hiding this comment

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

And, I don't see why it should use api.SearchError here, is the response really handled by frontend JS?

And, I don't see why it should use api.SearchError here, is the response really handled by frontend JS?

I think this needs a refactor to adjust so I don't think we should do that in this PR because this PR needs to be backport to v1.21.

Looks like it's a very old code. I sent a new commit 413b6c0 to return a simple error for non-admin users.

It doesn't look good to me if it would fill the code base with a lot of if ctx.Doer != nil && ctx.Doer.IsAdmin {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs a refactor to adjust so I don't think we should do that in this PR because this PR needs to be backport to v1.21.

Maybe it doesn't need to be backported since it is a (trivial?) performance optimization, not a bug fix or necessary enhancement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? It's not a big PR but will improve the dashboard performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think "because this PR needs to be backport to v1.21" is a strong reason to make the code more hacky.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 18, 2024
ctx.JSON(http.StatusInternalServerError, api.SearchError{
OK: false,
Error: err.Error(),
Error: getErrorStr(ctx, err, "SearchRepository internal error"),
Copy link
Contributor

Choose a reason for hiding this comment

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

getErrorStr makes the code more hacky. Would the code be filled with a lot of getErrorStr in every handler?

And I would ask the question a third time:

And, I don't see why it should use api.SearchError here, is the response really handled by frontend JS?

If I didn't misread the code, the frontend code NEVER handles the response error. It doens't need to respond anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

-> Only do counting when count_only=true for repo dashboard #29884

What do you think about this? It is simple enough and could be backported.

@lunny lunny closed this Mar 18, 2024
@lunny lunny deleted the lunny/remove_duplicated_repos_load branch March 18, 2024 10:41
lunny pushed a commit to lunny/gitea that referenced this pull request Mar 19, 2024
lunny added a commit that referenced this pull request Mar 20, 2024
…9905)

Ref: #29878
Backport #29884

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@lunny lunny removed the backport/v1.21 This PR should be backported to Gitea 1.21 label Mar 25, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code performance/speed performance issues with slow downs size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants