From 78517f80bbf797e21112a87e90c3286d9224d8bf Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 29 Apr 2024 08:23:35 +0800 Subject: [PATCH 1/3] Fix nil dereference on error (#30740) (#30746) Backport #30740 manually Co-authored-by: Chongyi Zheng (cherry picked from commit d07019f539c475466748fbf730d7f270a92fdb8b) --- routers/api/actions/artifacts.go | 7 ++++--- routers/web/repo/actions/view.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/routers/api/actions/artifacts.go b/routers/api/actions/artifacts.go index 2f8b3d0210..4306df5cd8 100644 --- a/routers/api/actions/artifacts.go +++ b/routers/api/actions/artifacts.go @@ -467,14 +467,15 @@ func (ar artifactRoutes) downloadArtifact(ctx *ArtifactContext) { log.Error("Error getting artifact: %v", err) ctx.Error(http.StatusInternalServerError, err.Error()) return - } else if !exist { + } + if !exist { log.Error("artifact with ID %d does not exist", artifactID) ctx.Error(http.StatusNotFound, fmt.Sprintf("artifact with ID %d does not exist", artifactID)) return } if artifact.RunID != runID { - log.Error("Error dismatch runID and artifactID, task: %v, artifact: %v", runID, artifactID) - ctx.Error(http.StatusBadRequest, err.Error()) + log.Error("Error mismatch runID and artifactID, task: %v, artifact: %v", runID, artifactID) + ctx.Error(http.StatusBadRequest) return } diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index fa687bbb93..6176ac1278 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -552,7 +552,7 @@ func getRunJobs(ctx *context_module.Context, runIndex, jobIndex int64) (*actions return nil, nil } if len(jobs) == 0 { - ctx.Error(http.StatusNotFound, err.Error()) + ctx.Error(http.StatusNotFound) return nil, nil } From 2da615c37cf781dc13bd00b93c08c42ab12eb052 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 30 Apr 2024 17:36:28 +0200 Subject: [PATCH 2/3] Get repo assignees and reviewers should ignore deactivated users (#30770) (#30782) Backport #30770 If an user is deactivated, it should not be in the list of users who are suggested to be assigned or review-requested. old assignees or reviewers are not affected. --- *Sponsored by Kithara Software GmbH* (cherry picked from commit 022eac4ac8e59f861237cc1e02f7ef117eaf8e30) Conflicts: models/repo/user_repo_test.go because there is one less fixture user compared to Gitea --- models/repo/user_repo.go | 8 ++++++-- models/repo/user_repo_test.go | 23 ++++++++++++++++------- tests/integration/api_repo_test.go | 4 +++- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index 219f4ff25d..330b1e5546 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -95,7 +95,10 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us // and just waste 1 unit is cheaper than re-allocate memory once. users := make([]*user_model.User, 0, len(uniqueUserIDs)+1) if len(userIDs) > 0 { - if err = e.In("id", uniqueUserIDs.Values()).OrderBy(user_model.GetOrderByName()).Find(&users); err != nil { + if err = e.In("id", uniqueUserIDs.Values()). + Where(builder.Eq{"`user`.is_active": true}). + OrderBy(user_model.GetOrderByName()). + Find(&users); err != nil { return nil, err } } @@ -117,7 +120,8 @@ func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) return nil, err } - cond := builder.And(builder.Neq{"`user`.id": posterID}) + cond := builder.And(builder.Neq{"`user`.id": posterID}). + And(builder.Eq{"`user`.is_active": true}) if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate { // This a private repository: diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index ad794beb9b..0433ff83d8 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -26,10 +26,17 @@ func TestRepoAssignees(t *testing.T) { repo21 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 21}) users, err = repo_model.GetRepoAssignees(db.DefaultContext, repo21) assert.NoError(t, err) - assert.Len(t, users, 3) - assert.Equal(t, users[0].ID, int64(15)) - assert.Equal(t, users[1].ID, int64(18)) - assert.Equal(t, users[2].ID, int64(16)) + if assert.Len(t, users, 3) { + assert.ElementsMatch(t, []int64{15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID}) + } + + // do not return deactivated users + assert.NoError(t, user_model.UpdateUserCols(db.DefaultContext, &user_model.User{ID: 15, IsActive: false}, "is_active")) + users, err = repo_model.GetRepoAssignees(db.DefaultContext, repo21) + assert.NoError(t, err) + if assert.Len(t, users, 2) { + assert.NotContains(t, []int64{users[0].ID, users[1].ID}, 15) + } } func TestRepoGetReviewers(t *testing.T) { @@ -41,17 +48,19 @@ func TestRepoGetReviewers(t *testing.T) { ctx := db.DefaultContext reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2) assert.NoError(t, err) - assert.Len(t, reviewers, 4) + if assert.Len(t, reviewers, 3) { + assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) + } // should include doer if doer is not PR poster. reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2) assert.NoError(t, err) - assert.Len(t, reviewers, 4) + assert.Len(t, reviewers, 3) // should not include PR poster, if PR poster would be otherwise eligible reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 4) assert.NoError(t, err) - assert.Len(t, reviewers, 3) + assert.Len(t, reviewers, 2) // test private user repo repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 2fb89cfa6e..96f38edc86 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -684,7 +684,9 @@ func TestAPIRepoGetReviewers(t *testing.T) { resp := MakeRequest(t, req, http.StatusOK) var reviewers []*api.User DecodeJSON(t, resp, &reviewers) - assert.Len(t, reviewers, 4) + if assert.Len(t, reviewers, 3) { + assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) + } } func TestAPIRepoGetAssignees(t *testing.T) { From 4fa80332e9b7f205b835616b346df3e072e28e5e Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 1 May 2024 10:10:37 +0800 Subject: [PATCH 3/3] Fix duplicate status check contexts (#30660) (#30779) Backport #30660. Caused by #30076. There may be some duplicate status check contexts when setting status checks for a branch protection rule. The duplicate contexts should be removed. Before: After: (cherry picked from commit 99e89e57bc4d2b3a3cd6c3068f818a0c7e8cec28) --- models/git/commit_status.go | 30 +++-------------- models/git/commit_status_test.go | 56 ++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 25 deletions(-) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index e44cc98985..45cf6c3c98 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -395,36 +395,16 @@ func GetLatestCommitStatusForRepoCommitIDs(ctx context.Context, repoID int64, co // FindRepoRecentCommitStatusContexts returns repository's recent commit status contexts func FindRepoRecentCommitStatusContexts(ctx context.Context, repoID int64, before time.Duration) ([]string, error) { - type result struct { - Index int64 - SHA string - } - getBase := func() *xorm.Session { - return db.GetEngine(ctx).Table(&CommitStatus{}).Where("repo_id = ?", repoID) - } - start := timeutil.TimeStampNow().AddDuration(-before) - results := make([]result, 0, 10) - sess := getBase().And("updated_unix >= ?", start). - Select("max( `index` ) as `index`, sha"). - GroupBy("context_hash, sha").OrderBy("max( `index` ) desc") - - err := sess.Find(&results) - if err != nil { + var contexts []string + if err := db.GetEngine(ctx).Table("commit_status"). + Where("repo_id = ?", repoID).And("updated_unix >= ?", start). + Cols("context").Distinct().Find(&contexts); err != nil { return nil, err } - contexts := make([]string, 0, len(results)) - if len(results) == 0 { - return contexts, nil - } - - conds := make([]builder.Cond, 0, len(results)) - for _, result := range results { - conds = append(conds, builder.Eq{"`index`": result.Index, "sha": result.SHA}) - } - return contexts, getBase().And(builder.Or(conds...)).Select("context").Find(&contexts) + return contexts, nil } // NewCommitStatusOptions holds options for creating a CommitStatus diff --git a/models/git/commit_status_test.go b/models/git/commit_status_test.go index 94c8d3776c..2ada8b3724 100644 --- a/models/git/commit_status_test.go +++ b/models/git/commit_status_test.go @@ -5,11 +5,15 @@ package git_test import ( "testing" + "time" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" @@ -183,3 +187,55 @@ func Test_CalcCommitStatus(t *testing.T) { assert.Equal(t, kase.expected, git_model.CalcCommitStatus(kase.statuses)) } } + +func TestFindRepoRecentCommitStatusContexts(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + gitRepo, err := gitrepo.OpenRepository(git.DefaultContext, repo2) + assert.NoError(t, err) + defer gitRepo.Close() + + commit, err := gitRepo.GetBranchCommit(repo2.DefaultBranch) + assert.NoError(t, err) + + defer func() { + _, err := db.DeleteByBean(db.DefaultContext, &git_model.CommitStatus{ + RepoID: repo2.ID, + CreatorID: user2.ID, + SHA: commit.ID.String(), + }) + assert.NoError(t, err) + }() + + err = git_model.NewCommitStatus(db.DefaultContext, git_model.NewCommitStatusOptions{ + Repo: repo2, + Creator: user2, + SHA: commit.ID, + CommitStatus: &git_model.CommitStatus{ + State: structs.CommitStatusFailure, + TargetURL: "https://example.com/tests/", + Context: "compliance/lint-backend", + }, + }) + assert.NoError(t, err) + + err = git_model.NewCommitStatus(db.DefaultContext, git_model.NewCommitStatusOptions{ + Repo: repo2, + Creator: user2, + SHA: commit.ID, + CommitStatus: &git_model.CommitStatus{ + State: structs.CommitStatusSuccess, + TargetURL: "https://example.com/tests/", + Context: "compliance/lint-backend", + }, + }) + assert.NoError(t, err) + + contexts, err := git_model.FindRepoRecentCommitStatusContexts(db.DefaultContext, repo2.ID, time.Hour) + assert.NoError(t, err) + if assert.Len(t, contexts, 1) { + assert.Equal(t, "compliance/lint-backend", contexts[0]) + } +}