From df4b87438023769877c85d446fa62b73896d5b6d Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 13 Mar 2024 11:10:16 +0800 Subject: [PATCH 1/2] fix --- models/webhook/webhook.go | 6 ++- modules/util/util.go | 9 ++++ services/webhook/deliver_test.go | 78 ++++++++++++-------------------- 3 files changed, 42 insertions(+), 51 deletions(-) diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index 894357e36a12..1e69ce97a3e4 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -150,8 +150,10 @@ func init() { // AfterLoad updates the webhook object upon setting a column func (w *Webhook) AfterLoad() { w.HookEvent = &webhook_module.HookEvent{} - if err := json.Unmarshal([]byte(w.Events), w.HookEvent); err != nil { - log.Error("Unmarshal[%d]: %v", w.ID, err) + if w.Events != "" { + if err := json.Unmarshal([]byte(w.Events), w.HookEvent); err != nil { + log.Error("Unmarshal[%d]: %v", w.ID, err) + } } } diff --git a/modules/util/util.go b/modules/util/util.go index 5c751581965c..c94fb910471c 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -212,3 +212,12 @@ func ToFloat64(number any) (float64, error) { func ToPointer[T any](val T) *T { return &val } + +// IfZero returns "def" if "v" is a zero value, otherwise "v" +func IfZero[T comparable](v, def T) T { + var zero T + if v == zero { + return def + } + return v +} diff --git a/services/webhook/deliver_test.go b/services/webhook/deliver_test.go index 24924ab214e0..85de1f99047e 100644 --- a/services/webhook/deliver_test.go +++ b/services/webhook/deliver_test.go @@ -18,6 +18,7 @@ import ( webhook_model "code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/modules/hostmatcher" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" webhook_module "code.gitea.io/gitea/modules/webhook" "github.com/stretchr/testify/assert" @@ -226,49 +227,29 @@ func TestWebhookDeliverSpecificTypes(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) type hookCase struct { - gotBody chan []byte + gotBody chan []byte + httpMethod string // default to POST } - cases := map[string]hookCase{ - webhook_module.SLACK: { - gotBody: make(chan []byte, 1), - }, - webhook_module.DISCORD: { - gotBody: make(chan []byte, 1), - }, - webhook_module.DINGTALK: { - gotBody: make(chan []byte, 1), - }, - webhook_module.TELEGRAM: { - gotBody: make(chan []byte, 1), - }, - webhook_module.MSTEAMS: { - gotBody: make(chan []byte, 1), - }, - webhook_module.FEISHU: { - gotBody: make(chan []byte, 1), - }, - webhook_module.MATRIX: { - gotBody: make(chan []byte, 1), - }, - webhook_module.WECHATWORK: { - gotBody: make(chan []byte, 1), - }, - webhook_module.PACKAGIST: { - gotBody: make(chan []byte, 1), - }, + cases := map[string]*hookCase{ + webhook_module.SLACK: {}, + webhook_module.DISCORD: {}, + webhook_module.DINGTALK: {}, + webhook_module.TELEGRAM: {}, + webhook_module.MSTEAMS: {}, + webhook_module.FEISHU: {}, + webhook_module.MATRIX: {httpMethod: "PUT"}, + webhook_module.WECHATWORK: {}, + webhook_module.PACKAGIST: {}, } s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + typ := strings.Split(r.URL.Path, "/")[1] // URL: "/{webhook_type}/other-path" assert.Equal(t, "application/json", r.Header.Get("Content-Type"), r.URL.Path) - - typ := strings.Split(r.URL.Path, "/")[1] // take first segment (after skipping leading slash) - hc := cases[typ] - require.NotNil(t, hc.gotBody, r.URL.Path) - body, err := io.ReadAll(r.Body) - assert.NoError(t, err) - w.WriteHeader(200) - hc.gotBody <- body + assert.Equal(t, util.IfZero(cases[typ].httpMethod, "POST"), r.Method, "webhook test request %q", r.URL.Path) + body, _ := io.ReadAll(r.Body) // read request and send it back to the test by testcase's chan + cases[typ].gotBody <- body + w.WriteHeader(http.StatusNoContent) })) t.Cleanup(s.Close) @@ -276,19 +257,17 @@ func TestWebhookDeliverSpecificTypes(t *testing.T) { data, err := p.JSONPayload() assert.NoError(t, err) - for typ, hc := range cases { - typ := typ - hc := hc + for typ := range cases { + cases[typ].gotBody = make(chan []byte, 1) + typ := typ // TODO: remove this workaround when Go >= 1.22 t.Run(typ, func(t *testing.T) { t.Parallel() hook := &webhook_model.Webhook{ - RepoID: 3, - IsActive: true, - Type: typ, - URL: s.URL + "/" + typ, - HTTPMethod: "POST", - ContentType: 0, // set to 0 so that falling back to default request fails with "invalid content type" - Meta: "{}", + RepoID: 3, + IsActive: true, + Type: typ, + URL: s.URL + "/" + typ, + Meta: "{}", } assert.NoError(t, webhook_model.CreateWebhook(db.DefaultContext, hook)) @@ -304,10 +283,11 @@ func TestWebhookDeliverSpecificTypes(t *testing.T) { assert.NotNil(t, hookTask) assert.NoError(t, Deliver(context.Background(), hookTask)) + select { - case gotBody := <-hc.gotBody: + case gotBody := <-cases[typ].gotBody: assert.NotEqual(t, string(data), string(gotBody), "request body must be different from the event payload") - assert.Equal(t, hookTask.RequestInfo.Body, string(gotBody), "request body was not saved") + assert.Equal(t, hookTask.RequestInfo.Body, string(gotBody), "delivered webhook payload doesn't match saved request") case <-time.After(5 * time.Second): t.Fatal("waited to long for request to happen") } From 3fc6c3e2634ae24ece3faebef14e6715fcce6b48 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 13 Mar 2024 16:14:10 +0800 Subject: [PATCH 2/2] Update webhook.go --- models/webhook/webhook.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index 1e69ce97a3e4..894357e36a12 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -150,10 +150,8 @@ func init() { // AfterLoad updates the webhook object upon setting a column func (w *Webhook) AfterLoad() { w.HookEvent = &webhook_module.HookEvent{} - if w.Events != "" { - if err := json.Unmarshal([]byte(w.Events), w.HookEvent); err != nil { - log.Error("Unmarshal[%d]: %v", w.ID, err) - } + if err := json.Unmarshal([]byte(w.Events), w.HookEvent); err != nil { + log.Error("Unmarshal[%d]: %v", w.ID, err) } }