Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for webhook and fix some webhook bugs (#33396) #33442

Merged
merged 6 commits into from
Feb 2, 2025
Merged

Add tests for webhook and fix some webhook bugs (#33396) #33442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b41bff7
Add tests for webhook and fix some webhook bugs (#33396)
lunny Jan 30, 2025
6a95a26
Fix status event
lunny Jan 30, 2025
4116d9d
Add a test for push_only webhook configuration
lunny Jan 30, 2025
6369446
Fix test
lunny Jan 30, 2025
f1f5184
Fix bug
lunny Jan 30, 2025
120751a
Merge branch 'release/v1.23' into lunny/add_webhook_test2
GiteaBot Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions models/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,11 @@ func (w *Webhook) HasPackageEvent() bool {
(w.ChooseEvents && w.HookEvents.Package)
}

func (w *Webhook) HasStatusEvent() bool {
return w.SendEverything ||
(w.ChooseEvents && w.HookEvents.Status)
}

// HasPullRequestReviewRequestEvent returns true if hook enabled pull request review request event.
func (w *Webhook) HasPullRequestReviewRequestEvent() bool {
return w.SendEverything ||
Expand Down Expand Up @@ -337,6 +342,7 @@ func (w *Webhook) EventCheckers() []struct {
{w.HasReleaseEvent, webhook_module.HookEventRelease},
{w.HasPackageEvent, webhook_module.HookEventPackage},
{w.HasPullRequestReviewRequestEvent, webhook_module.HookEventPullRequestReviewRequest},
{w.HasStatusEvent, webhook_module.HookEventStatus},
}
}

Expand Down
2 changes: 1 addition & 1 deletion models/webhook/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func TestWebhook_EventsArray(t *testing.T) {
"pull_request", "pull_request_assign", "pull_request_label", "pull_request_milestone",
"pull_request_comment", "pull_request_review_approved", "pull_request_review_rejected",
"pull_request_review_comment", "pull_request_sync", "wiki", "repository", "release",
"package", "pull_request_review_request",
"package", "pull_request_review_request", "status",
},
(&Webhook{
HookEvent: &webhook_module.HookEvent{SendEverything: true},
Expand Down
60 changes: 2 additions & 58 deletions modules/structs/hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,7 @@ var (
_ Payloader = &PackagePayload{}
)

// _________ __
// \_ ___ \_______ ____ _____ _/ |_ ____
// / \ \/\_ __ \_/ __ \\__ \\ __\/ __ \
// \ \____| | \/\ ___/ / __ \| | \ ___/
// \______ /|__| \___ >____ /__| \___ >
// \/ \/ \/ \/

// CreatePayload FIXME
// CreatePayload represents a payload information of create event.
type CreatePayload struct {
Sha string `json:"sha"`
Ref string `json:"ref"`
Expand Down Expand Up @@ -157,13 +150,6 @@ func ParseCreateHook(raw []byte) (*CreatePayload, error) {
return hook, nil
}

// ________ .__ __
// \______ \ ____ | | _____/ |_ ____
// | | \_/ __ \| | _/ __ \ __\/ __ \
// | ` \ ___/| |_\ ___/| | \ ___/
// /_______ /\___ >____/\___ >__| \___ >
// \/ \/ \/ \/

// PusherType define the type to push
type PusherType string

Expand All @@ -186,13 +172,6 @@ func (p *DeletePayload) JSONPayload() ([]byte, error) {
return json.MarshalIndent(p, "", " ")
}

// ___________ __
// \_ _____/__________| | __
// | __)/ _ \_ __ \ |/ /
// | \( <_> ) | \/ <
// \___ / \____/|__| |__|_ \
// \/ \/

// ForkPayload represents fork payload
type ForkPayload struct {
Forkee *Repository `json:"forkee"`
Expand Down Expand Up @@ -232,13 +211,6 @@ func (p *IssueCommentPayload) JSONPayload() ([]byte, error) {
return json.MarshalIndent(p, "", " ")
}

// __________ .__
// \______ \ ____ | | ____ _____ ______ ____
// | _// __ \| | _/ __ \\__ \ / ___// __ \
// | | \ ___/| |_\ ___/ / __ \_\___ \\ ___/
// |____|_ /\___ >____/\___ >____ /____ >\___ >
// \/ \/ \/ \/ \/ \/

// HookReleaseAction defines hook release action type
type HookReleaseAction string

Expand Down Expand Up @@ -302,13 +274,6 @@ func (p *PushPayload) Branch() string {
return strings.ReplaceAll(p.Ref, "refs/heads/", "")
}

// .___
// | | ______ ________ __ ____
// | |/ ___// ___/ | \_/ __ \
// | |\___ \ \___ \| | /\ ___/
// |___/____ >____ >____/ \___ >
// \/ \/ \/

// HookIssueAction FIXME
type HookIssueAction string

Expand Down Expand Up @@ -371,13 +336,6 @@ type ChangesPayload struct {
Ref *ChangesFromPayload `json:"ref,omitempty"`
}

// __________ .__ .__ __________ __
// \______ \__ __| | | | \______ \ ____ ________ __ ____ _______/ |_
// | ___/ | \ | | | | _// __ \/ ____/ | \_/ __ \ / ___/\ __\
// | | | | / |_| |__ | | \ ___< <_| | | /\ ___/ \___ \ | |
// |____| |____/|____/____/ |____|_ /\___ >__ |____/ \___ >____ > |__|
// \/ \/ |__| \/ \/

// PullRequestPayload represents a payload information of pull request event.
type PullRequestPayload struct {
Action HookIssueAction `json:"action"`
Expand All @@ -402,13 +360,6 @@ type ReviewPayload struct {
Content string `json:"content"`
}

// __ __.__ __ .__
// / \ / \__| | _|__|
// \ \/\/ / | |/ / |
// \ /| | <| |
// \__/\ / |__|__|_ \__|
// \/ \/

// HookWikiAction an action that happens to a wiki page
type HookWikiAction string

Expand All @@ -435,13 +386,6 @@ func (p *WikiPayload) JSONPayload() ([]byte, error) {
return json.MarshalIndent(p, "", " ")
}

//__________ .__ __
//\______ \ ____ ______ ____ _____|__|/ |_ ___________ ___.__.
// | _// __ \\____ \ / _ \/ ___/ \ __\/ _ \_ __ < | |
// | | \ ___/| |_> > <_> )___ \| || | ( <_> ) | \/\___ |
// |____|_ /\___ > __/ \____/____ >__||__| \____/|__| / ____|
// \/ \/|__| \/ \/

// HookRepoAction an action that happens to a repo
type HookRepoAction string

Expand Down Expand Up @@ -480,7 +424,7 @@ type PackagePayload struct {
Action HookPackageAction `json:"action"`
Repository *Repository `json:"repository"`
Package *Package `json:"package"`
Organization *User `json:"organization"`
Organization *Organization `json:"organization"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change?

Copy link
Member Author

@lunny lunny Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to consider it a bug of the previous implementation, not a feature change. So it's not a break change although it will make something break because the previous implementation is wrong. Compare the two structs,
User have many fields which should not belong to Organization, i.e.

    // Is the user an administrator
	IsAdmin bool `json:"is_admin"`
	// swagger:strfmt date-time
	LastLogin time.Time `json:"last_login,omitempty"`
    // Is user active
	IsActive bool `json:"active"`
	// Is user login prohibited
	ProhibitLogin bool `json:"prohibit_login"`
...

But I'm OK if you prefer to add a breaking label and description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yes, most useful fields are there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to backport, we need to mention that the struct has changed, to avoid wasting users' time

Sender *User `json:"sender"`
}

Expand Down
1 change: 1 addition & 0 deletions modules/webhook/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ type HookEvents struct {
Repository bool `json:"repository"`
Release bool `json:"release"`
Package bool `json:"package"`
Status bool `json:"status"`
}

// HookEvent represents events that will delivery hook.
Expand Down
17 changes: 2 additions & 15 deletions modules/webhook/type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,6 @@ const (
// Event returns the HookEventType as an event string
func (h HookEventType) Event() string {
switch h {
case HookEventCreate:
return "create"
case HookEventDelete:
return "delete"
case HookEventFork:
return "fork"
case HookEventPush:
return "push"
case HookEventIssues, HookEventIssueAssign, HookEventIssueLabel, HookEventIssueMilestone:
return "issues"
case HookEventPullRequest, HookEventPullRequestAssign, HookEventPullRequestLabel, HookEventPullRequestMilestone,
Expand All @@ -59,14 +51,9 @@ func (h HookEventType) Event() string {
return "pull_request_rejected"
case HookEventPullRequestReviewComment:
return "pull_request_comment"
case HookEventWiki:
return "wiki"
case HookEventRepository:
return "repository"
case HookEventRelease:
return "release"
default:
return string(h)
}
return ""
}

// HookType is the type of a webhook
Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/utils/hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI
Wiki: util.SliceContainsString(form.Events, string(webhook_module.HookEventWiki), true),
Repository: util.SliceContainsString(form.Events, string(webhook_module.HookEventRepository), true),
Release: util.SliceContainsString(form.Events, string(webhook_module.HookEventRelease), true),
Package: util.SliceContainsString(form.Events, string(webhook_module.HookEventPackage), true),
Status: util.SliceContainsString(form.Events, string(webhook_module.HookEventStatus), true),
},
BranchFilter: form.BranchFilter,
},
Expand Down
4 changes: 4 additions & 0 deletions services/webhook/dingtalk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,3 +190,7 @@ func newDingtalkRequest(_ context.Context, w *webhook_model.Webhook, t *webhook_
var pc payloadConvertor[DingtalkPayload] = dingtalkConvertor{}
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.DINGTALK, newDingtalkRequest)
}
4 changes: 4 additions & 0 deletions services/webhook/discord.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ func newDiscordRequest(_ context.Context, w *webhook_model.Webhook, t *webhook_m
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.DISCORD, newDiscordRequest)
}

func parseHookPullRequestEventType(event webhook_module.HookEventType) (string, error) {
switch event {
case webhook_module.HookEventPullRequestReviewApproved:
Expand Down
4 changes: 4 additions & 0 deletions services/webhook/feishu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,7 @@ func newFeishuRequest(_ context.Context, w *webhook_model.Webhook, t *webhook_mo
var pc payloadConvertor[FeishuPayload] = feishuConvertor{}
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.FEISHU, newFeishuRequest)
}
4 changes: 2 additions & 2 deletions services/webhook/general_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,8 @@ func packageTestPayload() *api.PackagePayload {
AvatarURL: "http://localhost:3000/user1/avatar",
},
Repository: nil,
Organization: &api.User{
UserName: "org1",
Organization: &api.Organization{
Name: "org1",
AvatarURL: "http://localhost:3000/org1/avatar",
},
Package: &api.Package{
Expand Down
4 changes: 4 additions & 0 deletions services/webhook/matrix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ import (
webhook_module "code.gitea.io/gitea/modules/webhook"
)

func init() {
RegisterWebhookRequester(webhook_module.MATRIX, newMatrixRequest)
}

func newMatrixRequest(_ context.Context, w *webhook_model.Webhook, t *webhook_model.HookTask) (*http.Request, []byte, error) {
meta := &MatrixMeta{}
if err := json.Unmarshal([]byte(w.Meta), meta); err != nil {
Expand Down
4 changes: 4 additions & 0 deletions services/webhook/msteams.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,3 +349,7 @@ func newMSTeamsRequest(_ context.Context, w *webhook_model.Webhook, t *webhook_m
var pc payloadConvertor[MSTeamsPayload] = msteamsConvertor{}
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.MSTEAMS, newMSTeamsRequest)
}
13 changes: 10 additions & 3 deletions services/webhook/notifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

git_model "code.gitea.io/gitea/models/git"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/organization"
packages_model "code.gitea.io/gitea/models/packages"
"code.gitea.io/gitea/models/perm"
access_model "code.gitea.io/gitea/models/perm/access"
Expand Down Expand Up @@ -924,10 +925,16 @@ func notifyPackage(ctx context.Context, sender *user_model.User, pd *packages_mo
return
}

var org *api.Organization
if pd.Owner.IsOrganization() {
org = convert.ToOrganization(ctx, organization.OrgFromUser(pd.Owner))
}

if err := PrepareWebhooks(ctx, source, webhook_module.HookEventPackage, &api.PackagePayload{
Action: action,
Package: apiPackage,
Sender: convert.ToUser(ctx, sender, nil),
Action: action,
Package: apiPackage,
Organization: org,
Sender: convert.ToUser(ctx, sender, nil),
}); err != nil {
log.Error("PrepareWebhooks: %v", err)
}
Expand Down
4 changes: 4 additions & 0 deletions services/webhook/packagist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,7 @@ func newPackagistRequest(_ context.Context, w *webhook_model.Webhook, t *webhook
}
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.PACKAGIST, newPackagistRequest)
}
4 changes: 4 additions & 0 deletions services/webhook/slack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,10 @@ func newSlackRequest(_ context.Context, w *webhook_model.Webhook, t *webhook_mod
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.SLACK, newSlackRequest)
}

var slackChannel = regexp.MustCompile(`^#?[a-z0-9_-]{1,80}$`)

// IsValidSlackChannel validates a channel name conforms to what slack expects:
Expand Down
4 changes: 4 additions & 0 deletions services/webhook/telegram.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,7 @@ func newTelegramRequest(_ context.Context, w *webhook_model.Webhook, t *webhook_
var pc payloadConvertor[TelegramPayload] = telegramConvertor{}
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.TELEGRAM, newTelegramRequest)
}
16 changes: 6 additions & 10 deletions services/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,12 @@ import (
"github.com/gobwas/glob"
)

var webhookRequesters = map[webhook_module.HookType]func(context.Context, *webhook_model.Webhook, *webhook_model.HookTask) (req *http.Request, body []byte, err error){
webhook_module.SLACK: newSlackRequest,
webhook_module.DISCORD: newDiscordRequest,
webhook_module.DINGTALK: newDingtalkRequest,
webhook_module.TELEGRAM: newTelegramRequest,
webhook_module.MSTEAMS: newMSTeamsRequest,
webhook_module.FEISHU: newFeishuRequest,
webhook_module.MATRIX: newMatrixRequest,
webhook_module.WECHATWORK: newWechatworkRequest,
webhook_module.PACKAGIST: newPackagistRequest,
type Requester func(context.Context, *webhook_model.Webhook, *webhook_model.HookTask) (req *http.Request, body []byte, err error)

var webhookRequesters = map[webhook_module.HookType]Requester{}

func RegisterWebhookRequester(hookType webhook_module.HookType, requester Requester) {
webhookRequesters[hookType] = requester
}

// IsValidHookTaskType returns true if a webhook registered
Expand Down
4 changes: 4 additions & 0 deletions services/webhook/wechatwork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,7 @@ func newWechatworkRequest(_ context.Context, w *webhook_model.Webhook, t *webhoo
var pc payloadConvertor[WechatworkPayload] = wechatworkConvertor{}
return newJSONRequest(pc, w, t, true)
}

func init() {
RegisterWebhookRequester(webhook_module.WECHATWORK, newWechatworkRequest)
}
15 changes: 10 additions & 5 deletions tests/integration/api_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,15 @@ func TestAPIMirrorSyncNonMirrorRepo(t *testing.T) {
assert.Equal(t, "Repository is not a mirror", errRespJSON["message"])
}

func testAPIOrgCreateRepo(t *testing.T, session *TestSession, orgName, repoName string, status int) {
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization, auth_model.AccessTokenScopeWriteRepository)

req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/org/%s/repos", orgName), &api.CreateRepoOption{
Name: repoName,
}).AddTokenAuth(token)
MakeRequest(t, req, status)
}

func TestAPIOrgRepoCreate(t *testing.T) {
testCases := []struct {
ctxUserID int64
Expand All @@ -488,11 +497,7 @@ func TestAPIOrgRepoCreate(t *testing.T) {
for _, testCase := range testCases {
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: testCase.ctxUserID})
session := loginUser(t, user.Name)
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization, auth_model.AccessTokenScopeWriteRepository)
req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/org/%s/repos", testCase.orgName), &api.CreateRepoOption{
Name: testCase.repoName,
}).AddTokenAuth(token)
MakeRequest(t, req, testCase.expectedStatus)
testAPIOrgCreateRepo(t, session, testCase.orgName, testCase.repoName, testCase.expectedStatus)
}
}

Expand Down
Loading