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.

Sign up for GitHub

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

Jump to bottom

Fix the duplicated repositories load on dashboard #29878

Closed
wants to merge 3 commits into from
Closed

Fix the duplicated repositories load on dashboard #29878

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69f54db
Fix the duplicated repositories load on dashboard
lunny Mar 18, 2024
413b6c0
Don't leak real error on search repositories
lunny Mar 18, 2024
06d5bac
Simple the return error
lunny Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions routers/web/repo/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,20 @@ func SearchRepo(ctx *context.Context) {
}
}

var err error
// To improve performance when only the count is requested
if ctx.FormBool("count_only") {
count, err := repo_model.CountRepository(ctx, opts)
if err != nil {
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 •
edited
Loading

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.

return
}
ctx.SetTotalCountHeader(count)
return
}

repos, count, err := repo_model.SearchRepository(ctx, opts)
if err != nil {
ctx.JSON(http.StatusInternalServerError, api.SearchError{
Expand All @@ -634,11 +647,6 @@ func SearchRepo(ctx *context.Context) {

ctx.SetTotalCountHeader(count)

// To improve performance when only the count is requested
if ctx.FormBool("count_only") {
return
}

latestCommitStatuses, err := commitstatus_service.FindReposLastestCommitStatuses(ctx, repos)
if err != nil {
log.Error("FindReposLastestCommitStatuses: %v", err)
Expand Down