Fix the duplicated repositories load on dashboard #29878
Conversation
lunny
added
performance/speed
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/S
GiteaBot
added
lgtm/need 1
routers/web/repo/repo.go
Outdated
| ctx.JSON(http.StatusInternalServerError, api.SearchError{ | ||
| OK: false, | ||
| Error: err.Error(), | ||
| }) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Looks like it's a very old code. I sent a new commit 413b6c0 to return a simple error for non-admin users.
There was a problem hiding this comment.
And, I don't see why it should use
api.SearchErrorhere, 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 {
There was a problem hiding this comment.
And, I don't see why it should use
api.SearchErrorhere, is the response really handled by frontend JS?And, I don't see why it should use
api.SearchErrorhere, 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 {
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why not? It's not a big PR but will improve the dashboard performance.
There was a problem hiding this comment.
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
bot
added
size/M
| ctx.JSON(http.StatusInternalServerError, api.SearchError{ | ||
| OK: false, | ||
| Error: err.Error(), | ||
| Error: getErrorStr(ctx, err, "SearchRepository internal error"), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
-> 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.
Even if there is
count_only, the previous code still invokedSearchRepositorywith 50 page size. That's an unnecessary waste. This PR will invokeCountRepositoryif there iscount_onlyparameter.