From aae19e6c19b8b8890b651b728693153da78e6cfa Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 16 May 2026 11:46:14 +0200 Subject: [PATCH] chore: tidy up uploading migration code (#12577) - Validate and sanitize topics. - Cap topics at 25 (limit used elsewhere, now unified constant). - Add more details and rephrase common user-facing error messages. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12577 --- models/repo/topic.go | 3 + routers/api/v1/repo/topic.go | 2 +- routers/web/repo/topic.go | 2 +- services/migrations/gitea_uploader.go | 28 +-- services/migrations/gitea_uploader_test.go | 188 ++++++++++++--------- 5 files changed, 123 insertions(+), 100 deletions(-) diff --git a/models/repo/topic.go b/models/repo/topic.go index 9086f17627..ea9866f007 100644 --- a/models/repo/topic.go +++ b/models/repo/topic.go @@ -20,6 +20,9 @@ func init() { db.RegisterModel(new(RepoTopic)) } +// MaxTopicsPerRepo caps the amount of topics allowed per repository. +const MaxTopicsPerRepo = 25 + var topicPattern = regexp.MustCompile(`^[a-z0-9][-.a-z0-9]*$`) // Topic represents a topic of repositories diff --git a/routers/api/v1/repo/topic.go b/routers/api/v1/repo/topic.go index 23fdab445b..51f1956888 100644 --- a/routers/api/v1/repo/topic.go +++ b/routers/api/v1/repo/topic.go @@ -104,7 +104,7 @@ func UpdateTopics(ctx *context.APIContext) { topicNames := form.Topics validTopics, invalidTopics := repo_model.SanitizeAndValidateTopics(topicNames) - if len(validTopics) > 25 { + if len(validTopics) > repo_model.MaxTopicsPerRepo { ctx.JSON(http.StatusUnprocessableEntity, map[string]any{ "invalidTopics": nil, "message": "Exceeding maximum number of topics per repo", diff --git a/routers/web/repo/topic.go b/routers/web/repo/topic.go index a028afb042..7b4bc48a94 100644 --- a/routers/web/repo/topic.go +++ b/routers/web/repo/topic.go @@ -29,7 +29,7 @@ func TopicsPost(ctx *context.Context) { validTopics, invalidTopics := repo_model.SanitizeAndValidateTopics(topics) - if len(validTopics) > 25 { + if len(validTopics) > repo_model.MaxTopicsPerRepo { ctx.JSON(http.StatusUnprocessableEntity, map[string]any{ "invalidTopics": nil, "message": ctx.Tr("repo.topic.count_prompt"), diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 7887dacdb1..566b86d706 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -161,18 +161,18 @@ func (g *GiteaLocalUploader) Close() { // CreateTopics creates topics func (g *GiteaLocalUploader) CreateTopics(topics ...string) error { - // Ignore topics too long for the db - c := 0 - for _, topic := range topics { - if len(topic) > 50 { - continue - } - - topics[c] = topic - c++ + validTopics, invalidTopics := repo_model.SanitizeAndValidateTopics(topics) + if len(invalidTopics) > 0 { + log.Warn("Invalid topics: %v in migration to %s/%s", invalidTopics, g.repoOwner, g.repoName) } - topics = topics[:c] - return repo_model.SaveTopics(g.ctx, g.repo.ID, topics...) + + // Truncate if it exceeds the maximum number of topics per repo. + if len(validTopics) > repo_model.MaxTopicsPerRepo { + validTopics = validTopics[:repo_model.MaxTopicsPerRepo] + log.Warn("More than %[0]d topics, truncated to first %[0]d topics in migration to %s/%s", repo_model.MaxTopicsPerRepo, g.repoOwner, g.repoName) + } + + return repo_model.SaveTopics(g.ctx, g.repo.ID, validTopics...) } // CreateMilestones creates milestones @@ -459,7 +459,7 @@ func (g *GiteaLocalUploader) CreateComments(comments ...*base.Comment) error { var issue *issues_model.Issue issue, ok := g.issues[comment.IssueIndex] if !ok { - return fmt.Errorf("comment references non existent IssueIndex %d", comment.IssueIndex) + return fmt.Errorf("comment[index=%d,poster_id=%d] belongs to a issue[index=%d] that was not migrated", comment.Index, comment.PosterID, comment.IssueIndex) } if comment.Created.IsZero() { @@ -566,7 +566,7 @@ func (g *GiteaLocalUploader) updateGitForPullRequest(pr *base.PullRequest) (head // SECURITY: this pr must have been must have been ensured safe if !pr.EnsuredSafe { log.Error("PR #%d in %s/%s has not been checked for safety.", pr.Number, g.repoOwner, g.repoName) - return "", fmt.Errorf("the PR[%d] was not checked for safety", pr.Number) + return "", fmt.Errorf("pull request[index=%d] was not checked for safety", pr.Number) } // Anonymous function to download the patch file (allows us to use defer) @@ -845,7 +845,7 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error { var issue *issues_model.Issue issue, ok := g.issues[review.IssueIndex] if !ok { - return fmt.Errorf("review references non existent IssueIndex %d", review.IssueIndex) + return fmt.Errorf("review[index=%d,reviewer_id=%d] belongs to a issue[index=%d] that was not migrated", review.ID, review.ReviewerID, review.IssueIndex) } if review.CreatedAt.IsZero() { review.CreatedAt = time.Unix(int64(issue.CreatedUnix), 0) diff --git a/services/migrations/gitea_uploader_test.go b/services/migrations/gitea_uploader_test.go index dc0210a8cc..0e6eceb784 100644 --- a/services/migrations/gitea_uploader_test.go +++ b/services/migrations/gitea_uploader_test.go @@ -29,7 +29,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestCommentUpload(t *testing.T) { +func TestUpload(t *testing.T) { unittest.PrepareTestEnv(t) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) var ( @@ -61,96 +61,116 @@ func TestCommentUpload(t *testing.T) { repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID, Name: repoName}) - // Create and Test Issues Uploading - issueA := &base.Issue{ - Title: "First issue", - Number: 0, - PosterID: 37243484, - PosterName: "PatDyn", - PosterEmail: "", - Content: "Mock Content", - Milestone: "Mock Milestone", - State: "open", - Created: time.Date(2025, 8, 7, 12, 44, 7, 0, time.UTC), - Updated: time.Date(2025, 8, 7, 12, 44, 47, 0, time.UTC), - Labels: nil, - Reactions: nil, - Closed: nil, - IsLocked: false, - Assignees: nil, - ForeignIndex: 0, - } + t.Run("Topic", func(t *testing.T) { + logChecker, cleanup := test.NewLogChecker(log.DEFAULT, log.INFO) + logChecker.StopMark("Invalid topics: [verylongtopicthatistoolongforgejomigration a b c d e f] in migration to user1/test_repo") + defer cleanup() - issueB := &base.Issue{ - Title: "Second Issue", - Number: 1, - PosterID: 37243484, - PosterName: "PatDyn", - PosterEmail: "", - Content: "Mock Content", - Milestone: "Mock Milestone", - State: "open", - Created: time.Date(2025, 8, 7, 12, 45, 44, 0, time.UTC), - Updated: time.Date(2025, 8, 7, 13, 7, 25, 0, time.UTC), - Labels: nil, - Reactions: nil, - Closed: nil, - IsLocked: false, - Assignees: nil, - ForeignIndex: 1, - } + require.NoError(t, uploader.CreateTopics("verylongtopicthatistoolongforgejomigration", "go ", " security", "a b c d e f")) - err := uploader.CreateIssues(issueA, issueB) - require.NoError(t, err) + _, logStopped := logChecker.Check(time.Second) + assert.True(t, logStopped) - issues, err := issues_model.Issues(db.DefaultContext, &issues_model.IssuesOptions{ - RepoIDs: []int64{repo.ID}, - IsPull: optional.Some(false), - SortType: "newest", + unittest.AssertExistsIf(t, false, &repo_model.Topic{Name: "verylongtopicthatistoolongforgejomigration"}) + unittest.AssertExistsIf(t, true, &repo_model.Topic{Name: "go"}) + unittest.AssertExistsIf(t, true, &repo_model.Topic{Name: "security"}) + unittest.AssertExistsIf(t, false, &repo_model.Topic{Name: "a b c d e f"}) }) - require.NoError(t, err) - assert.Len(t, issues, 2) - // Create and Test Comment Uploading - issueAComment := &base.Comment{ - IssueIndex: 0, - Index: 0, - CommentType: "comment", - PosterID: 37243484, - PosterName: "PatDyn", - PosterEmail: "", - Created: time.Date(2025, 8, 7, 12, 44, 24, 0, time.UTC), - Updated: time.Date(2025, 8, 7, 12, 44, 24, 0, time.UTC), - Content: "First Mock Comment", - Reactions: nil, - Meta: nil, - } - issueBComment := &base.Comment{ - IssueIndex: 1, - Index: 1, - CommentType: "comment", - PosterID: 37243484, - PosterName: "PatDyn", - PosterEmail: "", - Created: time.Date(2025, 8, 7, 13, 7, 25, 0, time.UTC), - Updated: time.Date(2025, 8, 7, 13, 7, 25, 0, time.UTC), - Content: "Second Mock Comment", - Reactions: nil, - Meta: nil, - } - require.NoError(t, uploader.CreateComments(issueBComment, issueAComment)) + t.Run("Issue", func(t *testing.T) { + // Create and Test Issues Uploading + issueA := &base.Issue{ + Title: "First issue", + Number: 0, + PosterID: 37243484, + PosterName: "PatDyn", + PosterEmail: "", + Content: "Mock Content", + Milestone: "Mock Milestone", + State: "open", + Created: time.Date(2025, 8, 7, 12, 44, 7, 0, time.UTC), + Updated: time.Date(2025, 8, 7, 12, 44, 47, 0, time.UTC), + Labels: nil, + Reactions: nil, + Closed: nil, + IsLocked: false, + Assignees: nil, + ForeignIndex: 0, + } - issues, err = issues_model.Issues(db.DefaultContext, &issues_model.IssuesOptions{ - RepoIDs: []int64{repo.ID}, - IsPull: optional.Some(false), - SortType: "newest", + issueB := &base.Issue{ + Title: "Second Issue", + Number: 1, + PosterID: 37243484, + PosterName: "PatDyn", + PosterEmail: "", + Content: "Mock Content", + Milestone: "Mock Milestone", + State: "open", + Created: time.Date(2025, 8, 7, 12, 45, 44, 0, time.UTC), + Updated: time.Date(2025, 8, 7, 13, 7, 25, 0, time.UTC), + Labels: nil, + Reactions: nil, + Closed: nil, + IsLocked: false, + Assignees: nil, + ForeignIndex: 1, + } + + err := uploader.CreateIssues(issueA, issueB) + require.NoError(t, err) + + issues, err := issues_model.Issues(db.DefaultContext, &issues_model.IssuesOptions{ + RepoIDs: []int64{repo.ID}, + IsPull: optional.Some(false), + SortType: "newest", + }) + require.NoError(t, err) + assert.Len(t, issues, 2) + }) + + t.Run("Comments", func(t *testing.T) { + // Create and Test Comment Uploading + issueAComment := &base.Comment{ + IssueIndex: 0, + Index: 0, + CommentType: "comment", + PosterID: 37243484, + PosterName: "PatDyn", + PosterEmail: "", + Created: time.Date(2025, 8, 7, 12, 44, 24, 0, time.UTC), + Updated: time.Date(2025, 8, 7, 12, 44, 24, 0, time.UTC), + Content: "First Mock Comment", + Reactions: nil, + Meta: nil, + } + issueBComment := &base.Comment{ + IssueIndex: 1, + Index: 1, + CommentType: "comment", + PosterID: 37243484, + PosterName: "PatDyn", + PosterEmail: "", + Created: time.Date(2025, 8, 7, 13, 7, 25, 0, time.UTC), + Updated: time.Date(2025, 8, 7, 13, 7, 25, 0, time.UTC), + Content: "Second Mock Comment", + Reactions: nil, + Meta: nil, + } + require.NoError(t, uploader.CreateComments(issueBComment, issueAComment)) + + issues, err := issues_model.Issues(db.DefaultContext, &issues_model.IssuesOptions{ + RepoIDs: []int64{repo.ID}, + IsPull: optional.Some(false), + SortType: "newest", + }) + require.NoError(t, err) + assert.Len(t, issues, 2) + require.NoError(t, issues[0].LoadDiscussComments(db.DefaultContext)) + require.NoError(t, issues[1].LoadDiscussComments(db.DefaultContext)) + assert.Len(t, issues[0].Comments, 1) + assert.Len(t, issues[1].Comments, 1) }) - require.NoError(t, err) - assert.Len(t, issues, 2) - require.NoError(t, issues[0].LoadDiscussComments(db.DefaultContext)) - require.NoError(t, issues[1].LoadDiscussComments(db.DefaultContext)) - assert.Len(t, issues[0].Comments, 1) - assert.Len(t, issues[1].Comments, 1) } func TestGiteaUploadRepo(t *testing.T) {