Skip to content

Commit 9484502

Browse files
GustedGusted
Gusted
authored and
Gusted
committed
feat: add commit limit for webhook payload (#6797)
- Adds a new option `[webhook].PAYLOAD_COMMIT_LIMIT` that limits the amount of commits is sent for each webhook payload, this was previously done via `[ui].FEED_MAX_COMMIT_NUM` which feels incorrect. - The default is 15 for this new option, purely arbitary. - Resolves forgejo/forgejo#6780 - Added unit testing, it's quite a lot because this the notification area is not really easy to test and rather should've been a integration test but that ends up having more complicated than trying doing an unit test. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6797 Reviewed-by: Otto <[email protected]> Reviewed-by: 0ko <[email protected]> Co-authored-by: Gusted <[email protected]> Co-committed-by: Gusted <[email protected]>
1 parent 93f84db commit 9484502

File tree

7 files changed

+268
-18
lines changed

7 files changed

+268
-18
lines changed

‎modules/setting/webhook.go

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,21 +11,23 @@ import (
1111

1212
// Webhook settings
1313
var Webhook = struct {
14-
QueueLength int
15-
DeliverTimeout int
16-
SkipTLSVerify bool
17-
AllowedHostList string
18-
PagingNum int
19-
ProxyURL string
20-
ProxyURLFixed *url.URL
21-
ProxyHosts []string
14+
QueueLength int
15+
DeliverTimeout int
16+
SkipTLSVerify bool
17+
AllowedHostList string
18+
PagingNum int
19+
ProxyURL string
20+
ProxyURLFixed *url.URL
21+
ProxyHosts []string
22+
PayloadCommitLimit int
2223
}{
23-
QueueLength: 1000,
24-
DeliverTimeout: 5,
25-
SkipTLSVerify: false,
26-
PagingNum: 10,
27-
ProxyURL: "",
28-
ProxyHosts: []string{},
24+
QueueLength: 1000,
25+
DeliverTimeout: 5,
26+
SkipTLSVerify: false,
27+
PagingNum: 10,
28+
ProxyURL: "",
29+
ProxyHosts: []string{},
30+
PayloadCommitLimit: 15,
2931
}
3032

3133
func loadWebhookFrom(rootCfg ConfigProvider) {
@@ -45,4 +47,5 @@ func loadWebhookFrom(rootCfg ConfigProvider) {
4547
}
4648
}
4749
Webhook.ProxyHosts = sec.Key("PROXY_HOSTS").Strings(",")
50+
Webhook.PayloadCommitLimit = sec.Key("PAYLOAD_COMMIT_LIMIT").MustInt(15)
4851
}

‎services/feed/action.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"code.gitea.io/gitea/modules/json"
1818
"code.gitea.io/gitea/modules/log"
1919
"code.gitea.io/gitea/modules/repository"
20+
"code.gitea.io/gitea/modules/setting"
2021
"code.gitea.io/gitea/modules/util"
2122
notify_service "code.gitea.io/gitea/services/notify"
2223
)
@@ -319,6 +320,10 @@ func (*actionNotifier) NotifyPullRevieweDismiss(ctx context.Context, doer *user_
319320
}
320321

321322
func (a *actionNotifier) PushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) {
323+
if len(commits.Commits) > setting.UI.FeedMaxCommitNum {
324+
commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum]
325+
}
326+
322327
data, err := json.Marshal(commits)
323328
if err != nil {
324329
log.Error("Marshal: %v", err)
@@ -390,6 +395,10 @@ func (a *actionNotifier) DeleteRef(ctx context.Context, doer *user_model.User, r
390395
}
391396

392397
func (a *actionNotifier) SyncPushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) {
398+
if len(commits.Commits) > setting.UI.FeedMaxCommitNum {
399+
commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum]
400+
}
401+
393402
data, err := json.Marshal(commits)
394403
if err != nil {
395404
log.Error("json.Marshal: %v", err)

‎services/feed/action_test.go

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,15 @@ import (
1212
repo_model "code.gitea.io/gitea/models/repo"
1313
"code.gitea.io/gitea/models/unittest"
1414
user_model "code.gitea.io/gitea/models/user"
15+
"code.gitea.io/gitea/modules/git"
16+
"code.gitea.io/gitea/modules/repository"
17+
"code.gitea.io/gitea/modules/setting"
18+
"code.gitea.io/gitea/modules/test"
1519

1620
_ "code.gitea.io/gitea/models/actions"
1721
_ "code.gitea.io/gitea/models/forgefed"
1822

23+
"github.com/stretchr/testify/assert"
1924
"github.com/stretchr/testify/require"
2025
)
2126

@@ -51,3 +56,89 @@ func TestRenameRepoAction(t *testing.T) {
5156
unittest.AssertExistsAndLoadBean(t, actionBean)
5257
unittest.CheckConsistencyFor(t, &activities_model.Action{})
5358
}
59+
60+
func pushCommits() *repository.PushCommits {
61+
pushCommits := repository.NewPushCommits()
62+
pushCommits.Commits = []*repository.PushCommit{
63+
{
64+
Sha1: "69554a6",
65+
CommitterEmail: "[email protected]",
66+
CommitterName: "User2",
67+
AuthorEmail: "[email protected]",
68+
AuthorName: "User2",
69+
Message: "not signed commit",
70+
},
71+
{
72+
Sha1: "27566bd",
73+
CommitterEmail: "[email protected]",
74+
CommitterName: "User2",
75+
AuthorEmail: "[email protected]",
76+
AuthorName: "User2",
77+
Message: "good signed commit (with not yet validated email)",
78+
},
79+
{
80+
Sha1: "5099b81",
81+
CommitterEmail: "[email protected]",
82+
CommitterName: "User2",
83+
AuthorEmail: "[email protected]",
84+
AuthorName: "User2",
85+
Message: "good signed commit",
86+
},
87+
}
88+
pushCommits.HeadCommit = &repository.PushCommit{Sha1: "69554a6"}
89+
return pushCommits
90+
}
91+
92+
func TestSyncPushCommits(t *testing.T) {
93+
require.NoError(t, unittest.PrepareTestDatabase())
94+
95+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
96+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID})
97+
98+
t.Run("All commits", func(t *testing.T) {
99+
defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 10)()
100+
101+
maxID := unittest.GetCount(t, &activities_model.Action{})
102+
NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master")}, pushCommits())
103+
104+
newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/master"}, unittest.Cond("id > ?", maxID))
105+
assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"27566bd","Message":"good signed commit (with not yet validated email)","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"5099b81","Message":"good signed commit","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content)
106+
})
107+
108+
t.Run("Only one commit", func(t *testing.T) {
109+
defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 1)()
110+
111+
maxID := unittest.GetCount(t, &activities_model.Action{})
112+
NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main")}, pushCommits())
113+
114+
newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/main"}, unittest.Cond("id > ?", maxID))
115+
assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content)
116+
})
117+
}
118+
119+
func TestPushCommits(t *testing.T) {
120+
require.NoError(t, unittest.PrepareTestDatabase())
121+
122+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
123+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: user.ID})
124+
125+
t.Run("All commits", func(t *testing.T) {
126+
defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 10)()
127+
128+
maxID := unittest.GetCount(t, &activities_model.Action{})
129+
NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master")}, pushCommits())
130+
131+
newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/master"}, unittest.Cond("id > ?", maxID))
132+
assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"27566bd","Message":"good signed commit (with not yet validated email)","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"},{"Sha1":"5099b81","Message":"good signed commit","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content)
133+
})
134+
135+
t.Run("Only one commit", func(t *testing.T) {
136+
defer test.MockVariableValue(&setting.UI.FeedMaxCommitNum, 1)()
137+
138+
maxID := unittest.GetCount(t, &activities_model.Action{})
139+
NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main")}, pushCommits())
140+
141+
newNotification := unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ActUserID: user.ID, RefName: "refs/heads/main"}, unittest.Cond("id > ?", maxID))
142+
assert.JSONEq(t, `{"Commits":[{"Sha1":"69554a6","Message":"not signed commit","AuthorEmail":"[email protected]","AuthorName":"User2","CommitterEmail":"[email protected]","CommitterName":"User2","Timestamp":"0001-01-01T00:00:00Z"}],"HeadCommit":{"Sha1":"69554a6","Message":"","AuthorEmail":"","AuthorName":"","CommitterEmail":"","CommitterName":"","Timestamp":"0001-01-01T00:00:00Z"},"CompareURL":"","Len":0}`, newNotification.Content)
143+
})
144+
}

‎services/repository/push.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
252252
commits.CompareURL = ""
253253
}
254254

255-
if len(commits.Commits) > setting.UI.FeedMaxCommitNum {
256-
commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum]
257-
}
258-
259255
notify_service.PushCommits(ctx, pusher, repo, opts, commits)
260256

261257
// Cache for big repository

‎services/webhook/TestPushCommits/webhook.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
-
2+
id: 1001
3+
repo_id: 2
4+
type: forgejo
5+
url: http://www.example.com/blåhaj
6+
http_method: POST
7+
content_type: 1 # json
8+
events: '{"send_everything":true,"branch_filter":"{master*,main*}"}'
9+
is_active: true

‎services/webhook/notifier.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,10 @@ func (m *webhookNotifier) IssueChangeMilestone(ctx context.Context, doer *user_m
599599
}
600600

601601
func (m *webhookNotifier) PushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) {
602+
if len(commits.Commits) > setting.Webhook.PayloadCommitLimit {
603+
commits.Commits = commits.Commits[:setting.Webhook.PayloadCommitLimit]
604+
}
605+
602606
apiPusher := convert.ToUser(ctx, pusher, nil)
603607
apiCommits, apiHeadCommit, err := commits.ToAPIPayloadCommits(ctx, repo.RepoPath(), repo.HTMLURL())
604608
if err != nil {
@@ -840,6 +844,10 @@ func (m *webhookNotifier) DeleteRelease(ctx context.Context, doer *user_model.Us
840844
}
841845

842846
func (m *webhookNotifier) SyncPushCommits(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) {
847+
if len(commits.Commits) > setting.Webhook.PayloadCommitLimit {
848+
commits.Commits = commits.Commits[:setting.Webhook.PayloadCommitLimit]
849+
}
850+
843851
apiPusher := convert.ToUser(ctx, pusher, nil)
844852
apiCommits, apiHeadCommit, err := commits.ToAPIPayloadCommits(ctx, repo.RepoPath(), repo.HTMLURL())
845853
if err != nil {

‎services/webhook/notifier_test.go

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
// Copyright 2025 The Forgejo Authors. All rights reserved.
2+
// SPDX-License-Identifier: GPL-3.0-or-later
3+
4+
package webhook
5+
6+
import (
7+
"path/filepath"
8+
"testing"
9+
10+
"code.gitea.io/gitea/models/db"
11+
repo_model "code.gitea.io/gitea/models/repo"
12+
"code.gitea.io/gitea/models/unittest"
13+
user_model "code.gitea.io/gitea/models/user"
14+
webhook_model "code.gitea.io/gitea/models/webhook"
15+
"code.gitea.io/gitea/modules/git"
16+
"code.gitea.io/gitea/modules/json"
17+
"code.gitea.io/gitea/modules/repository"
18+
"code.gitea.io/gitea/modules/setting"
19+
"code.gitea.io/gitea/modules/structs"
20+
"code.gitea.io/gitea/modules/test"
21+
22+
"github.com/stretchr/testify/assert"
23+
"github.com/stretchr/testify/require"
24+
)
25+
26+
func pushCommits() *repository.PushCommits {
27+
pushCommits := repository.NewPushCommits()
28+
pushCommits.Commits = []*repository.PushCommit{
29+
{
30+
Sha1: "2c54faec6c45d31c1abfaecdab471eac6633738a",
31+
CommitterEmail: "[email protected]",
32+
CommitterName: "User2",
33+
AuthorEmail: "[email protected]",
34+
AuthorName: "User2",
35+
Message: "not signed commit",
36+
},
37+
{
38+
Sha1: "205ac761f3326a7ebe416e8673760016450b5cec",
39+
CommitterEmail: "[email protected]",
40+
CommitterName: "User2",
41+
AuthorEmail: "[email protected]",
42+
AuthorName: "User2",
43+
Message: "good signed commit (with not yet validated email)",
44+
},
45+
{
46+
Sha1: "1032bbf17fbc0d9c95bb5418dabe8f8c99278700",
47+
CommitterEmail: "[email protected]",
48+
CommitterName: "User2",
49+
AuthorEmail: "[email protected]",
50+
AuthorName: "User2",
51+
Message: "good signed commit",
52+
},
53+
}
54+
pushCommits.HeadCommit = &repository.PushCommit{Sha1: "2c54faec6c45d31c1abfaecdab471eac6633738a"}
55+
return pushCommits
56+
}
57+
58+
func TestSyncPushCommits(t *testing.T) {
59+
defer unittest.OverrideFixtures(
60+
unittest.FixturesOptions{
61+
Dir: filepath.Join(setting.AppWorkPath, "models/fixtures/"),
62+
Base: setting.AppWorkPath,
63+
Dirs: []string{"services/webhook/TestPushCommits"},
64+
},
65+
)()
66+
require.NoError(t, unittest.PrepareTestDatabase())
67+
68+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
69+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: user.ID})
70+
71+
t.Run("All commits", func(t *testing.T) {
72+
defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 10)()
73+
74+
NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master-1")}, pushCommits())
75+
76+
hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%master-1%'"))
77+
78+
var payloadContent structs.PushPayload
79+
require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent))
80+
assert.Len(t, payloadContent.Commits, 3)
81+
})
82+
83+
t.Run("Only one commit", func(t *testing.T) {
84+
defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 1)()
85+
86+
NewNotifier().SyncPushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main-1")}, pushCommits())
87+
88+
hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%main-1%'"))
89+
90+
var payloadContent structs.PushPayload
91+
require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent))
92+
assert.Len(t, payloadContent.Commits, 1)
93+
assert.EqualValues(t, "2c54faec6c45d31c1abfaecdab471eac6633738a", payloadContent.Commits[0].ID)
94+
})
95+
}
96+
97+
func TestPushCommits(t *testing.T) {
98+
defer unittest.OverrideFixtures(
99+
unittest.FixturesOptions{
100+
Dir: filepath.Join(setting.AppWorkPath, "models/fixtures/"),
101+
Base: setting.AppWorkPath,
102+
Dirs: []string{"services/webhook/TestPushCommits"},
103+
},
104+
)()
105+
require.NoError(t, unittest.PrepareTestDatabase())
106+
107+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
108+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: user.ID})
109+
110+
t.Run("All commits", func(t *testing.T) {
111+
defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 10)()
112+
113+
NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("master-2")}, pushCommits())
114+
115+
hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%master-2%'"))
116+
117+
var payloadContent structs.PushPayload
118+
require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent))
119+
assert.Len(t, payloadContent.Commits, 3)
120+
})
121+
122+
t.Run("Only one commit", func(t *testing.T) {
123+
defer test.MockVariableValue(&setting.Webhook.PayloadCommitLimit, 1)()
124+
125+
NewNotifier().PushCommits(db.DefaultContext, user, repo, &repository.PushUpdateOptions{RefFullName: git.RefNameFromBranch("main-2")}, pushCommits())
126+
127+
hookTask := unittest.AssertExistsAndLoadBean(t, &webhook_model.HookTask{}, unittest.Cond("payload_content LIKE '%main-2%'"))
128+
129+
var payloadContent structs.PushPayload
130+
require.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payloadContent))
131+
assert.Len(t, payloadContent.Commits, 1)
132+
assert.EqualValues(t, "2c54faec6c45d31c1abfaecdab471eac6633738a", payloadContent.Commits[0].ID)
133+
})
134+
}

0 commit comments

Comments
 (0)