diff --git a/models/activities/action.go b/models/activities/action.go index f928ad6784..5067109545 100644 --- a/models/activities/action.go +++ b/models/activities/action.go @@ -26,6 +26,7 @@ import ( "forgejo.org/modules/base" "forgejo.org/modules/container" "forgejo.org/modules/git" + "forgejo.org/modules/json" "forgejo.org/modules/log" "forgejo.org/modules/setting" "forgejo.org/modules/structs" @@ -386,8 +387,21 @@ func (a *Action) IsIssueEvent() bool { // GetIssueInfos returns a list of associated information with the action. func (a *Action) GetIssueInfos() []string { + // Previously multiple pieces of data used to be encoded into a.Content by pipe-separating them, but this doesn't + // work well if some of the user-entered pieces of content (issue titles, comments, etc.) contain pipes. The newer + // storage format is to json-encode a string array, which we check for and prefer... then fallback to assuming old. + var ret []string + if strings.HasPrefix(a.Content, "[") && strings.HasSuffix(a.Content, "]") { + ret = make([]string, 0, 3) + err := json.Unmarshal([]byte(a.Content), &ret) + if err != nil { + log.Error("GetIssueInfos json decoding error: %v", err) + } + } else { + ret = strings.SplitN(a.Content, "|", 3) + } + // make sure it always returns 3 elements, because there are some access to the a[1] and a[2] without checking the length - ret := strings.SplitN(a.Content, "|", 3) for len(ret) < 3 { ret = append(ret, "") } @@ -770,7 +784,9 @@ func DeleteIssueActions(ctx context.Context, repoID, issueID, issueIndex int64) _, err := e.Where("repo_id = ?", repoID). In("op_type", ActionCreateIssue, ActionCreatePullRequest). - Where("content LIKE ?", strconv.FormatInt(issueIndex, 10)+"|%"). // "IssueIndex|content..." + Where(builder.Or( + builder.Like{"content", strconv.FormatInt(issueIndex, 10) + "|%"}, // "IssueIndex|content..." + builder.Like{"content", "[\"" + strconv.FormatInt(issueIndex, 10) + "\"%"})). // JSON, ["IssueIndex"... Delete(&Action{}) return err } diff --git a/models/activities/action_test.go b/models/activities/action_test.go index 161d05bbfa..6911733a09 100644 --- a/models/activities/action_test.go +++ b/models/activities/action_test.go @@ -308,14 +308,69 @@ func TestDeleteIssueActions(t *testing.T) { }) require.NoError(t, err) err = db.Insert(db.DefaultContext, &activities_model.Action{ - OpType: activities_model.ActionCreateIssue, - RepoID: issue.RepoID, + OpType: activities_model.ActionCreateIssue, + RepoID: issue.RepoID, + // Older Content format... Content: fmt.Sprintf("%d|content...", issue.Index), }) require.NoError(t, err) + err = db.Insert(db.DefaultContext, &activities_model.Action{ + OpType: activities_model.ActionCreateIssue, + RepoID: issue.RepoID, + // JSON-encoded Content format... + Content: fmt.Sprintf("[\"%d\",\"content...\"]", issue.Index), + }) + require.NoError(t, err) // assert that the actions exist, then delete them - unittest.AssertCount(t, &activities_model.Action{}, 2) + unittest.AssertCount(t, &activities_model.Action{}, 3) require.NoError(t, activities_model.DeleteIssueActions(db.DefaultContext, issue.RepoID, issue.ID, issue.Index)) unittest.AssertCount(t, &activities_model.Action{}, 0) } + +func TestGetIssueInfos(t *testing.T) { + tt := []struct { + content string + field1 string + field2 string + field3 string + }{ + { + content: "4|", + field1: "4", + }, + { + content: "2|docs: Add README w/ template sections", + field1: "2", + field2: "docs: Add README w/ template sections", + }, + { + content: "2|docs: Add README w/ template sections|Some comment...", + field1: "2", + field2: "docs: Add README w/ template sections", + field3: "Some comment...", + }, + { + content: "[\"4\"]", + field1: "4", + }, + { + content: "[\"2\", \"docs: Add README w/ | template sections\"]", + field1: "2", + field2: "docs: Add README w/ | template sections", + }, + { + content: "[\"2\", \"docs: Add README w/ | template sections\", \"Some | comment...\"]", + field1: "2", + field2: "docs: Add README w/ | template sections", + field3: "Some | comment...", + }, + } + for _, test := range tt { + action := &activities_model.Action{Content: test.content} + issueInfos := action.GetIssueInfos() + assert.Equal(t, test.field1, issueInfos[0]) + assert.Equal(t, test.field2, issueInfos[1]) + assert.Equal(t, test.field3, issueInfos[2]) + } +} diff --git a/services/feed/action.go b/services/feed/action.go index c708ae5404..e945e0905e 100644 --- a/services/feed/action.go +++ b/services/feed/action.go @@ -71,7 +71,7 @@ func (a *actionNotifier) NewIssue(ctx context.Context, issue *issues_model.Issue ActUserID: issue.Poster.ID, ActUser: issue.Poster, OpType: activities_model.ActionCreateIssue, - Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title), + Content: encodeContent(fmt.Sprintf("%d", issue.Index), issue.Title), RepoID: repo.ID, Repo: repo, IsPrivate: repo.IsPrivate, @@ -87,7 +87,7 @@ func (a *actionNotifier) IssueChangeStatus(ctx context.Context, doer *user_model act := &activities_model.Action{ ActUserID: doer.ID, ActUser: doer, - Content: fmt.Sprintf("%d|%s", issue.Index, ""), + Content: encodeContent(fmt.Sprintf("%d", issue.Index), ""), RepoID: issue.Repo.ID, Repo: issue.Repo, Comment: actionComment, @@ -135,7 +135,7 @@ func (a *actionNotifier) CreateIssueComment(ctx context.Context, doer *user_mode truncatedContent = truncatedContent[:lastSpaceIdx] + "…" } } - act.Content = fmt.Sprintf("%d|%s", issue.Index, truncatedContent) + act.Content = encodeContent(fmt.Sprintf("%d", issue.Index), truncatedContent) if issue.IsPull { act.OpType = activities_model.ActionCommentPull @@ -167,7 +167,7 @@ func (a *actionNotifier) NewPullRequest(ctx context.Context, pull *issues_model. ActUserID: pull.Issue.Poster.ID, ActUser: pull.Issue.Poster, OpType: activities_model.ActionCreatePullRequest, - Content: fmt.Sprintf("%d|%s", pull.Issue.Index, pull.Issue.Title), + Content: encodeContent(fmt.Sprintf("%d", pull.Issue.Index), pull.Issue.Title), RepoID: pull.Issue.Repo.ID, Repo: pull.Issue.Repo, IsPrivate: pull.Issue.Repo.IsPrivate, @@ -247,7 +247,7 @@ func (a *actionNotifier) PullRequestReview(ctx context.Context, pr *issues_model actions = append(actions, &activities_model.Action{ ActUserID: review.Reviewer.ID, ActUser: review.Reviewer, - Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comm.Content, "\n")[0]), + Content: encodeContent(fmt.Sprintf("%d", review.Issue.Index), strings.Split(comm.Content, "\n")[0]), OpType: activities_model.ActionCommentPull, RepoID: review.Issue.RepoID, Repo: review.Issue.Repo, @@ -263,7 +263,7 @@ func (a *actionNotifier) PullRequestReview(ctx context.Context, pr *issues_model action := &activities_model.Action{ ActUserID: review.Reviewer.ID, ActUser: review.Reviewer, - Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comment.Content, "\n")[0]), + Content: encodeContent(fmt.Sprintf("%d", review.Issue.Index), strings.Split(comment.Content, "\n")[0]), RepoID: review.Issue.RepoID, Repo: review.Issue.Repo, IsPrivate: review.Issue.Repo.IsPrivate, @@ -293,7 +293,7 @@ func (*actionNotifier) MergePullRequest(ctx context.Context, doer *user_model.Us ActUserID: doer.ID, ActUser: doer, OpType: activities_model.ActionMergePullRequest, - Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title), + Content: encodeContent(fmt.Sprintf("%d", pr.Issue.Index), pr.Issue.Title), RepoID: pr.Issue.Repo.ID, Repo: pr.Issue.Repo, IsPrivate: pr.Issue.Repo.IsPrivate, @@ -307,7 +307,7 @@ func (*actionNotifier) AutoMergePullRequest(ctx context.Context, doer *user_mode ActUserID: doer.ID, ActUser: doer, OpType: activities_model.ActionAutoMergePullRequest, - Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title), + Content: encodeContent(fmt.Sprintf("%d", pr.Issue.Index), pr.Issue.Title), RepoID: pr.Issue.Repo.ID, Repo: pr.Issue.Repo, IsPrivate: pr.Issue.Repo.IsPrivate, @@ -325,7 +325,7 @@ func (*actionNotifier) NotifyPullRevieweDismiss(ctx context.Context, doer *user_ ActUserID: doer.ID, ActUser: doer, OpType: activities_model.ActionPullReviewDismissed, - Content: fmt.Sprintf("%d|%s|%s", review.Issue.Index, reviewerName, comment.Content), + Content: encodeContent(fmt.Sprintf("%d", review.Issue.Index), reviewerName, comment.Content), RepoID: review.Issue.Repo.ID, Repo: review.Issue.Repo, IsPrivate: review.Issue.Repo.IsPrivate, @@ -482,3 +482,12 @@ func (a *actionNotifier) NewRelease(ctx context.Context, rel *repo_model.Release log.Error("NotifyWatchers: %v", err) } } + +// ... later decoded in models/activities/action.go:GetIssueInfos +func encodeContent(params ...string) string { + contentEncoded, err := json.Marshal(params) + if err != nil { + log.Error("encodeContent: Unexpected json encoding error: %v", err) + } + return string(contentEncoded) +} diff --git a/tests/integration/feed_user_test.go b/tests/integration/feed_user_test.go index dfcb33b35d..c914e15f69 100644 --- a/tests/integration/feed_user_test.go +++ b/tests/integration/feed_user_test.go @@ -76,6 +76,7 @@ func TestFeed(t *testing.T) { data := resp.Body.String() assert.Contains(t, data, `