From 0161f3019b0fe7afc512e793acdfcf58ad079e55 Mon Sep 17 00:00:00 2001 From: JoeGruffins <34998433+JoeGruffins@users.noreply.github.com> Date: Sat, 18 Apr 2026 06:54:49 +0900 Subject: [PATCH] pull: Fix CODEOWNERS absolute path matching. (#37244) Patterns starting with "/" (e.g. /docs/.*\.md) never matched because git returns relative paths without a leading slash. Strip the leading "/" before compiling the regex since the ^...$ anchoring already provides root-relative semantics. Fixes: #28107 Co-authored-by: silverwind Co-authored-by: Giteabot Co-authored-by: wxiaoguang --- models/issues/pull.go | 7 +- models/issues/pull_test.go | 132 +++++++++++++++++++++++-------------- 2 files changed, 89 insertions(+), 50 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index c07044f3011..1b883f29810 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -877,7 +877,12 @@ func ParseCodeOwnersLine(ctx context.Context, tokens []string) (*CodeOwnerRule, warnings := make([]string, 0) - expr := fmt.Sprintf("^%s$", strings.TrimPrefix(tokens[0], "!")) + // Strip leading "!" for negative rules, then strip leading "/" since + // git returns relative paths (e.g. "docs/foo.md" not "/docs/foo.md") + // and the regex is already anchored with ^...$, so the "/" is redundant. + pattern := strings.TrimPrefix(tokens[0], "!") + pattern = strings.TrimPrefix(pattern, "/") + expr := fmt.Sprintf("^%s$", pattern) rule.Rule, err = regexp2.Compile(expr, regexp2.None) if err != nil { warnings = append(warnings, fmt.Sprintf("incorrect codeowner regexp: %s", err)) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 25b27cbe9c9..79d1f8aa9b4 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -17,16 +17,43 @@ import ( "github.com/stretchr/testify/require" ) -func TestPullRequest_LoadAttributes(t *testing.T) { +func TestPullRequest(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("LoadAttributes", testPullRequestLoadAttributes) + t.Run("LoadIssue", testPullRequestLoadIssue) + t.Run("LoadBaseRepo", testPullRequestLoadBaseRepo) + t.Run("LoadHeadRepo", testPullRequestLoadHeadRepo) + t.Run("PullRequestsNewest", testPullRequestsNewest) + t.Run("PullRequestsOldest", testPullRequestsOldest) + t.Run("GetUnmergedPullRequest", testGetUnmergedPullRequest) + t.Run("HasUnmergedPullRequestsByHeadInfo", testHasUnmergedPullRequestsByHeadInfo) + t.Run("GetUnmergedPullRequestsByHeadInfo", testGetUnmergedPullRequestsByHeadInfo) + t.Run("GetUnmergedPullRequestsByBaseInfo", testGetUnmergedPullRequestsByBaseInfo) + t.Run("GetPullRequestByIndex", testGetPullRequestByIndex) + t.Run("GetPullRequestByID", testGetPullRequestByID) + t.Run("GetPullRequestByIssueID", testGetPullRequestByIssueID) + t.Run("PullRequest_UpdateCols", testPullRequestUpdateCols) + t.Run("PullRequest_IsWorkInProgress", testPullRequestIsWorkInProgress) + t.Run("PullRequest_GetWorkInProgressPrefixWorkInProgress", testPullRequestGetWorkInProgressPrefixWorkInProgress) + t.Run("DeleteOrphanedObjects", testDeleteOrphanedObjects) + t.Run("ParseCodeOwnersLine", testParseCodeOwnersLine) + t.Run("CodeOwnerAbsolutePathPatterns", testCodeOwnerAbsolutePathPatterns) + t.Run("GetApprovers", testGetApprovers) + t.Run("GetPullRequestByMergedCommit", testGetPullRequestByMergedCommit) + t.Run("Migrate_InsertPullRequests", testMigrateInsertPullRequests) + t.Run("PullRequestsClosedRecentSortType", testPullRequestsClosedRecentSortType) + t.Run("LoadRequestedReviewers", testLoadRequestedReviewers) +} + +func testPullRequestLoadAttributes(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) assert.NoError(t, pr.LoadAttributes(t.Context())) assert.NotNil(t, pr.Merger) assert.Equal(t, pr.MergerID, pr.Merger.ID) } -func TestPullRequest_LoadIssue(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testPullRequestLoadIssue(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) assert.NoError(t, pr.LoadIssue(t.Context())) assert.NotNil(t, pr.Issue) @@ -36,8 +63,7 @@ func TestPullRequest_LoadIssue(t *testing.T) { assert.Equal(t, int64(2), pr.Issue.ID) } -func TestPullRequest_LoadBaseRepo(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testPullRequestLoadBaseRepo(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) assert.NoError(t, pr.LoadBaseRepo(t.Context())) assert.NotNil(t, pr.BaseRepo) @@ -47,8 +73,7 @@ func TestPullRequest_LoadBaseRepo(t *testing.T) { assert.Equal(t, pr.BaseRepoID, pr.BaseRepo.ID) } -func TestPullRequest_LoadHeadRepo(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testPullRequestLoadHeadRepo(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) assert.NoError(t, pr.LoadHeadRepo(t.Context())) assert.NotNil(t, pr.HeadRepo) @@ -59,8 +84,7 @@ func TestPullRequest_LoadHeadRepo(t *testing.T) { // TODO TestNewPullRequest -func TestPullRequestsNewest(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testPullRequestsNewest(t *testing.T) { prs, count, err := issues_model.PullRequests(t.Context(), 1, &issues_model.PullRequestsOptions{ ListOptions: db.ListOptions{ Page: 1, @@ -77,7 +101,7 @@ func TestPullRequestsNewest(t *testing.T) { } } -func TestPullRequests_Closed_RecentSortType(t *testing.T) { +func testPullRequestsClosedRecentSortType(t *testing.T) { // Issue ID | Closed At. | Updated At // 2 | 1707270001 | 1707270001 // 3 | 1707271000 | 1707279999 @@ -90,7 +114,6 @@ func TestPullRequests_Closed_RecentSortType(t *testing.T) { {"recentclose", []int64{11, 3, 2}}, } - assert.NoError(t, unittest.PrepareTestDatabase()) _, err := db.Exec(t.Context(), "UPDATE issue SET closed_unix = 1707270001, updated_unix = 1707270001, is_closed = true WHERE id = 2") require.NoError(t, err) _, err = db.Exec(t.Context(), "UPDATE issue SET closed_unix = 1707271000, updated_unix = 1707279999, is_closed = true WHERE id = 3") @@ -118,9 +141,7 @@ func TestPullRequests_Closed_RecentSortType(t *testing.T) { } } -func TestLoadRequestedReviewers(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - +func testLoadRequestedReviewers(t *testing.T) { pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) assert.NoError(t, pull.LoadIssue(t.Context())) issue := pull.Issue @@ -146,8 +167,7 @@ func TestLoadRequestedReviewers(t *testing.T) { assert.Empty(t, pull.RequestedReviewers) } -func TestPullRequestsOldest(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testPullRequestsOldest(t *testing.T) { prs, count, err := issues_model.PullRequests(t.Context(), 1, &issues_model.PullRequestsOptions{ ListOptions: db.ListOptions{ Page: 1, @@ -164,8 +184,7 @@ func TestPullRequestsOldest(t *testing.T) { } } -func TestGetUnmergedPullRequest(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testGetUnmergedPullRequest(t *testing.T) { pr, err := issues_model.GetUnmergedPullRequest(t.Context(), 1, 1, "branch2", "master", issues_model.PullRequestFlowGithub) assert.NoError(t, err) assert.Equal(t, int64(2), pr.ID) @@ -175,9 +194,7 @@ func TestGetUnmergedPullRequest(t *testing.T) { assert.True(t, issues_model.IsErrPullRequestNotExist(err)) } -func TestHasUnmergedPullRequestsByHeadInfo(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - +func testHasUnmergedPullRequestsByHeadInfo(t *testing.T) { exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(t.Context(), 1, "branch2") assert.NoError(t, err) assert.True(t, exist) @@ -187,8 +204,7 @@ func TestHasUnmergedPullRequestsByHeadInfo(t *testing.T) { assert.False(t, exist) } -func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testGetUnmergedPullRequestsByHeadInfo(t *testing.T) { prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(t.Context(), 1, "branch2") assert.NoError(t, err) assert.Len(t, prs, 1) @@ -198,8 +214,7 @@ func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) { } } -func TestGetUnmergedPullRequestsByBaseInfo(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testGetUnmergedPullRequestsByBaseInfo(t *testing.T) { prs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(t.Context(), 1, "master") assert.NoError(t, err) assert.Len(t, prs, 1) @@ -209,8 +224,7 @@ func TestGetUnmergedPullRequestsByBaseInfo(t *testing.T) { assert.Equal(t, "master", pr.BaseBranch) } -func TestGetPullRequestByIndex(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testGetPullRequestByIndex(t *testing.T) { pr, err := issues_model.GetPullRequestByIndex(t.Context(), 1, 2) assert.NoError(t, err) assert.Equal(t, int64(1), pr.BaseRepoID) @@ -225,8 +239,7 @@ func TestGetPullRequestByIndex(t *testing.T) { assert.True(t, issues_model.IsErrPullRequestNotExist(err)) } -func TestGetPullRequestByID(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testGetPullRequestByID(t *testing.T) { pr, err := issues_model.GetPullRequestByID(t.Context(), 1) assert.NoError(t, err) assert.Equal(t, int64(1), pr.ID) @@ -237,8 +250,7 @@ func TestGetPullRequestByID(t *testing.T) { assert.True(t, issues_model.IsErrPullRequestNotExist(err)) } -func TestGetPullRequestByIssueID(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testGetPullRequestByIssueID(t *testing.T) { pr, err := issues_model.GetPullRequestByIssueID(t.Context(), 2) assert.NoError(t, err) assert.Equal(t, int64(2), pr.IssueID) @@ -248,8 +260,7 @@ func TestGetPullRequestByIssueID(t *testing.T) { assert.True(t, issues_model.IsErrPullRequestNotExist(err)) } -func TestPullRequest_UpdateCols(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testPullRequestUpdateCols(t *testing.T) { pr := &issues_model.PullRequest{ ID: 1, BaseBranch: "baseBranch", @@ -265,9 +276,7 @@ func TestPullRequest_UpdateCols(t *testing.T) { // TODO TestAddTestPullRequestTask -func TestPullRequest_IsWorkInProgress(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - +func testPullRequestIsWorkInProgress(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) pr.LoadIssue(t.Context()) @@ -280,9 +289,7 @@ func TestPullRequest_IsWorkInProgress(t *testing.T) { assert.True(t, pr.IsWorkInProgress(t.Context())) } -func TestPullRequest_GetWorkInProgressPrefixWorkInProgress(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - +func testPullRequestGetWorkInProgressPrefixWorkInProgress(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) pr.LoadIssue(t.Context()) @@ -296,9 +303,7 @@ func TestPullRequest_GetWorkInProgressPrefixWorkInProgress(t *testing.T) { assert.Equal(t, "[wip]", pr.GetWorkInProgressPrefix(t.Context())) } -func TestDeleteOrphanedObjects(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - +func testDeleteOrphanedObjects(t *testing.T) { countBefore, err := db.GetEngine(t.Context()).Count(&issues_model.PullRequest{}) assert.NoError(t, err) @@ -317,7 +322,7 @@ func TestDeleteOrphanedObjects(t *testing.T) { assert.Equal(t, countBefore, countAfter) } -func TestParseCodeOwnersLine(t *testing.T) { +func testParseCodeOwnersLine(t *testing.T) { type CodeOwnerTest struct { Line string Tokens []string @@ -331,6 +336,8 @@ func TestParseCodeOwnersLine(t *testing.T) { {Line: `docs/(aws|google|azure)/[^/]*\\.(md|txt) @org3 @org2/team2`, Tokens: []string{`docs/(aws|google|azure)/[^/]*\.(md|txt)`, "@org3", "@org2/team2"}}, {Line: `\#path @org3`, Tokens: []string{`#path`, "@org3"}}, {Line: `path\ with\ spaces/ @org3`, Tokens: []string{`path with spaces/`, "@org3"}}, + {Line: `/docs/.*\\.md @user1`, Tokens: []string{`/docs/.*\.md`, "@user1"}}, + {Line: `!/assets/.*\\.(bin|exe|msi) @user1`, Tokens: []string{`!/assets/.*\.(bin|exe|msi)`, "@user1"}}, } for _, g := range given { @@ -339,8 +346,37 @@ func TestParseCodeOwnersLine(t *testing.T) { } } -func TestGetApprovers(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testCodeOwnerAbsolutePathPatterns(t *testing.T) { + type testCase struct { + content string + file string + expected bool + } + + cases := []testCase{ + // Absolute path pattern should match (leading "/" stripped) + {content: "/README.md @user5\n", file: "README.md", expected: true}, + // Absolute path pattern in subdirectory + {content: "/docs/.* @user5\n", file: "docs/foo.md", expected: true}, + // Absolute path should not match nested paths it shouldn't + {content: "/docs/.* @user5\n", file: "other/docs/foo.md", expected: false}, + // Relative path still works + {content: "README.md @user5\n", file: "README.md", expected: true}, + // Negated absolute path pattern + {content: "!/.* @user5\n", file: "README.md", expected: false}, + } + + for _, c := range cases { + rules, _ := issues_model.GetCodeOwnersFromContent(t.Context(), c.content) + require.NotEmpty(t, rules) + rule := rules[0] + regexpMatched, _ := rule.Rule.MatchString(c.file) + ruleMatched := regexpMatched == !rule.Negative + assert.Equal(t, c.expected, ruleMatched, "pattern %q against file %q", c.content, c.file) + } +} + +func testGetApprovers(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5}) // Official reviews are already deduplicated. Allow unofficial reviews // to assert that there are no duplicated approvers. @@ -350,8 +386,7 @@ func TestGetApprovers(t *testing.T) { assert.Equal(t, expected, approvers) } -func TestGetPullRequestByMergedCommit(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testGetPullRequestByMergedCommit(t *testing.T) { pr, err := issues_model.GetPullRequestByMergedCommit(t.Context(), 1, "1a8823cd1a9549fde083f992f6b9b87a7ab74fb3") assert.NoError(t, err) assert.EqualValues(t, 1, pr.ID) @@ -362,8 +397,7 @@ func TestGetPullRequestByMergedCommit(t *testing.T) { assert.ErrorAs(t, err, &issues_model.ErrPullRequestNotExist{}) } -func TestMigrate_InsertPullRequests(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) +func testMigrateInsertPullRequests(t *testing.T) { reponame := "repo1" repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: reponame}) owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})