From 5dddcc1773b9245a7bcd1f71a83a0681ea541538 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 3 Feb 2025 02:16:56 +0800 Subject: [PATCH] chore: fix some trivial problems and TODOs (#33473) 1. Fix incorrect `MentionCount` (actually it seems to be deadcode, affects nothing) 2. Remove fallback sha1 support for time limit token 3. Use route middleware `reqRepoActionsWriter` for `ArtifactsDeleteView` 4. Use clearer message "Failed to authenticate user" instead of "Verify" when auth fails 5. `tests/integration/benchmarks_test.go` is not quite right, actually it is never used, so delete it. 6. Remove or update TODO comments --- models/issues/issue_stats.go | 2 +- models/system/notice_test.go | 2 - modules/base/tool.go | 9 +--- modules/base/tool_test.go | 11 ++--- modules/markup/html.go | 2 +- modules/web/middleware/binding.go | 2 +- routers/web/repo/actions/view.go | 5 -- routers/web/web.go | 4 +- services/mailer/mail_test.go | 16 +++---- tests/integration/benchmarks_test.go | 69 ---------------------------- 10 files changed, 18 insertions(+), 104 deletions(-) delete mode 100644 tests/integration/benchmarks_test.go diff --git a/models/issues/issue_stats.go b/models/issues/issue_stats.go index 9ef9347a16c5a..50409fbbd895d 100644 --- a/models/issues/issue_stats.go +++ b/models/issues/issue_stats.go @@ -107,7 +107,7 @@ func GetIssueStats(ctx context.Context, opts *IssuesOptions) (*IssueStats, error accum.YourRepositoriesCount += stats.YourRepositoriesCount accum.AssignCount += stats.AssignCount accum.CreateCount += stats.CreateCount - accum.OpenCount += stats.MentionCount + accum.MentionCount += stats.MentionCount accum.ReviewRequestedCount += stats.ReviewRequestedCount accum.ReviewedCount += stats.ReviewedCount i = chunk diff --git a/models/system/notice_test.go b/models/system/notice_test.go index 599b2fb65c325..9fc9e6cce1936 100644 --- a/models/system/notice_test.go +++ b/models/system/notice_test.go @@ -45,8 +45,6 @@ func TestCreateRepositoryNotice(t *testing.T) { unittest.AssertExistsAndLoadBean(t, noticeBean) } -// TODO TestRemoveAllWithNotice - func TestCountNotices(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) assert.Equal(t, int64(3), system.CountNotices(db.DefaultContext)) diff --git a/modules/base/tool.go b/modules/base/tool.go index 1d16186bc5d01..b6ed8cbf9a5a7 100644 --- a/modules/base/tool.go +++ b/modules/base/tool.go @@ -18,7 +18,6 @@ import ( "time" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" @@ -64,10 +63,7 @@ func VerifyTimeLimitCode(now time.Time, data string, minutes int, code string) b // check code retCode := CreateTimeLimitCode(data, aliveTime, startTimeStr, nil) if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 { - retCode = CreateTimeLimitCode(data, aliveTime, startTimeStr, sha1.New()) // TODO: this is only for the support of legacy codes, remove this in/after 1.23 - if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 { - return false - } + return false } // check time is expired or not: startTime <= now && now < startTime + minutes @@ -144,13 +140,12 @@ func Int64sToStrings(ints []int64) []string { return strs } -// EntryIcon returns the octicon class for displaying files/directories +// EntryIcon returns the octicon name for displaying files/directories func EntryIcon(entry *git.TreeEntry) string { switch { case entry.IsLink(): te, err := entry.FollowLink() if err != nil { - log.Debug(err.Error()) return "file-symlink-file" } if te.IsDir() { diff --git a/modules/base/tool_test.go b/modules/base/tool_test.go index c821a55c19c0e..7cebedb073ceb 100644 --- a/modules/base/tool_test.go +++ b/modules/base/tool_test.go @@ -86,13 +86,10 @@ JWT_SECRET = %s verifyDataCode := func(c string) bool { return VerifyTimeLimitCode(now, "data", 2, c) } - code1 := CreateTimeLimitCode("data", 2, now, sha1.New()) - code2 := CreateTimeLimitCode("data", 2, now, nil) - assert.True(t, verifyDataCode(code1)) - assert.True(t, verifyDataCode(code2)) + code := CreateTimeLimitCode("data", 2, now, nil) + assert.True(t, verifyDataCode(code)) initGeneralSecret("000_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko") - assert.False(t, verifyDataCode(code1)) - assert.False(t, verifyDataCode(code2)) + assert.False(t, verifyDataCode(code)) }) } @@ -137,5 +134,3 @@ func TestInt64sToStrings(t *testing.T) { Int64sToStrings([]int64{1, 4, 16, 64, 256}), ) } - -// TODO: Test EntryIcon diff --git a/modules/markup/html.go b/modules/markup/html.go index bb12febf277ec..3aaf669c63267 100644 --- a/modules/markup/html.go +++ b/modules/markup/html.go @@ -47,7 +47,7 @@ var globalVars = sync.OnceValue(func() *globalVarsType { // NOTE: All below regex matching do not perform any extra validation. // Thus a link is produced even if the linked entity does not exist. // While fast, this is also incorrect and lead to false positives. - // TODO: fix invalid linking issue + // TODO: fix invalid linking issue (update: stale TODO, what issues? maybe no TODO anymore) // valid chars in encoded path and parameter: [-+~_%.a-zA-Z0-9/] diff --git a/modules/web/middleware/binding.go b/modules/web/middleware/binding.go index 43e1bbc70e3b5..03e188f5098b4 100644 --- a/modules/web/middleware/binding.go +++ b/modules/web/middleware/binding.go @@ -78,7 +78,7 @@ func GetInclude(field reflect.StructField) string { return getRuleBody(field, "Include(") } -// Validate validate TODO: +// Validate validate func Validate(errs binding.Errors, data map[string]any, f Form, l translation.Locale) binding.Errors { if errs.Len() == 0 { return errs diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index e9321d0a8a1cd..fc346b83b4736 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -628,11 +628,6 @@ func getRunJobs(ctx *context_module.Context, runIndex, jobIndex int64) (*actions } func ArtifactsDeleteView(ctx *context_module.Context) { - if !ctx.Repo.CanWrite(unit.TypeActions) { - ctx.Error(http.StatusForbidden, "no permission") - return - } - runIndex := getRunIndex(ctx) artifactName := ctx.PathParam("artifact_name") diff --git a/routers/web/web.go b/routers/web/web.go index daba9887e8f14..f772f6dbb979d 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -118,7 +118,7 @@ func webAuth(authMethod auth_service.Method) func(*context.Context) { ar, err := common.AuthShared(ctx.Base, ctx.Session, authMethod) if err != nil { log.Error("Failed to verify user: %v", err) - ctx.Error(http.StatusUnauthorized, "Verify") + ctx.Error(http.StatusUnauthorized, "Failed to authenticate user") return } ctx.Doer = ar.Doer @@ -1430,7 +1430,7 @@ func registerRoutes(m *web.Router) { m.Post("/cancel", reqRepoActionsWriter, actions.Cancel) m.Post("/approve", reqRepoActionsWriter, actions.Approve) m.Get("/artifacts/{artifact_name}", actions.ArtifactsDownloadView) - m.Delete("/artifacts/{artifact_name}", actions.ArtifactsDeleteView) + m.Delete("/artifacts/{artifact_name}", reqRepoActionsWriter, actions.ArtifactsDeleteView) m.Post("/rerun", reqRepoActionsWriter, actions.Rerun) }) m.Group("/workflows/{workflow_name}", func() { diff --git a/services/mailer/mail_test.go b/services/mailer/mail_test.go index 36cef486c94fa..8298ac4a34281 100644 --- a/services/mailer/mail_test.go +++ b/services/mailer/mail_test.go @@ -85,7 +85,7 @@ func TestComposeIssueCommentMessage(t *testing.T) { recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}, {Name: "Test2", Email: "test2@gitea.com"}} msgs, err := composeIssueCommentMessages(&mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context + Context: context.TODO(), Issue: issue, Doer: doer, ActionType: activities_model.ActionCommentIssue, Content: fmt.Sprintf("test @%s %s#%d body", doer.Name, issue.Repo.FullName(), issue.Index), Comment: comment, @@ -131,7 +131,7 @@ func TestComposeIssueMessage(t *testing.T) { recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}, {Name: "Test2", Email: "test2@gitea.com"}} msgs, err := composeIssueCommentMessages(&mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context + Context: context.TODO(), Issue: issue, Doer: doer, ActionType: activities_model.ActionCreateIssue, Content: "test body", }, "en-US", recipients, false, "issue create") @@ -178,14 +178,14 @@ func TestTemplateSelection(t *testing.T) { } msg := testComposeIssueCommentMessage(t, &mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context + Context: context.TODO(), Issue: issue, Doer: doer, ActionType: activities_model.ActionCreateIssue, Content: "test body", }, recipients, false, "TestTemplateSelection") expect(t, msg, "issue/new/subject", "issue/new/body") msg = testComposeIssueCommentMessage(t, &mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context + Context: context.TODO(), Issue: issue, Doer: doer, ActionType: activities_model.ActionCommentIssue, Content: "test body", Comment: comment, }, recipients, false, "TestTemplateSelection") @@ -194,14 +194,14 @@ func TestTemplateSelection(t *testing.T) { pull := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2, Repo: repo, Poster: doer}) comment = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4, Issue: pull}) msg = testComposeIssueCommentMessage(t, &mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context + Context: context.TODO(), Issue: pull, Doer: doer, ActionType: activities_model.ActionCommentPull, Content: "test body", Comment: comment, }, recipients, false, "TestTemplateSelection") expect(t, msg, "pull/comment/subject", "pull/comment/body") msg = testComposeIssueCommentMessage(t, &mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context + Context: context.TODO(), Issue: issue, Doer: doer, ActionType: activities_model.ActionCloseIssue, Content: "test body", Comment: comment, }, recipients, false, "TestTemplateSelection") @@ -220,7 +220,7 @@ func TestTemplateServices(t *testing.T) { recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}} msg := testComposeIssueCommentMessage(t, &mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context + Context: context.TODO(), Issue: issue, Doer: doer, ActionType: actionType, Content: "test body", Comment: comment, }, recipients, fromMention, "TestTemplateServices") @@ -263,7 +263,7 @@ func testComposeIssueCommentMessage(t *testing.T, ctx *mailCommentContext, recip func TestGenerateAdditionalHeaders(t *testing.T) { doer, _, issue, _ := prepareMailerTest(t) - ctx := &mailCommentContext{Context: context.TODO() /* TODO: use a correct context */, Issue: issue, Doer: doer} + ctx := &mailCommentContext{Context: context.TODO(), Issue: issue, Doer: doer} recipient := &user_model.User{Name: "test", Email: "test@gitea.com"} headers := generateAdditionalHeaders(ctx, "dummy-reason", recipient) diff --git a/tests/integration/benchmarks_test.go b/tests/integration/benchmarks_test.go deleted file mode 100644 index 62da761d2dcd7..0000000000000 --- a/tests/integration/benchmarks_test.go +++ /dev/null @@ -1,69 +0,0 @@ -// Copyright 2017 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package integration - -import ( - "math/rand/v2" - "net/http" - "net/url" - "testing" - - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unittest" - api "code.gitea.io/gitea/modules/structs" -) - -// StringWithCharset random string (from https://www.calhoun.io/creating-random-strings-in-go/) -func StringWithCharset(length int, charset string) string { - b := make([]byte, length) - for i := range b { - b[i] = charset[rand.IntN(len(charset))] - } - return string(b) -} - -func BenchmarkRepoBranchCommit(b *testing.B) { - onGiteaRun(b, func(b *testing.B, u *url.URL) { - samples := []int64{1, 2, 3} - b.ResetTimer() - - for _, repoID := range samples { - b.StopTimer() - repo := unittest.AssertExistsAndLoadBean(b, &repo_model.Repository{ID: repoID}) - b.StartTimer() - b.Run(repo.Name, func(b *testing.B) { - session := loginUser(b, "user2") - b.ResetTimer() - b.Run("CreateBranch", func(b *testing.B) { - b.StopTimer() - branchName := StringWithCharset(5+rand.IntN(10), "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") - b.StartTimer() - for i := 0; i < b.N; i++ { - b.Run("new_"+branchName, func(b *testing.B) { - b.Skip("benchmark broken") // TODO fix - testAPICreateBranch(b, session, repo.OwnerName, repo.Name, repo.DefaultBranch, "new_"+branchName, http.StatusCreated) - }) - } - }) - b.Run("GetBranches", func(b *testing.B) { - req := NewRequestf(b, "GET", "/api/v1/repos/%s/branches", repo.FullName()) - session.MakeRequest(b, req, http.StatusOK) - }) - b.Run("AccessCommits", func(b *testing.B) { - var branches []*api.Branch - req := NewRequestf(b, "GET", "/api/v1/repos/%s/branches", repo.FullName()) - resp := session.MakeRequest(b, req, http.StatusOK) - DecodeJSON(b, resp, &branches) - b.ResetTimer() // We measure from here - if len(branches) != 0 { - for i := 0; i < b.N; i++ { - req := NewRequestf(b, "GET", "/api/v1/repos/%s/commits?sha=%s", repo.FullName(), branches[i%len(branches)].Name) - session.MakeRequest(b, req, http.StatusOK) - } - } - }) - }) - } - }) -}