fix(api): do not duplicate payload in build params for hooks (#4956)

engine/api/workflow/process_node.go

@@ -508,8 +508,8 @@ func computeBuildParameters(wr *sdk.WorkflowRun, run *sdk.WorkflowNodeRun, paren
 		Value: run.WorkflowNodeName,
 	})
 
-	// ADD PAYLOAD as STRING
-	if run.Payload != nil {
+	// ADD PAYLOAD as STRING only for manual run
+	if run.Payload != nil && run.Manual != nil {
 		payloadStr, err := json.Marshal(run.Payload)
 		if err != nil {
 			return nil, sdk.WrapError(err, "unable to marshal payload")

engine/api/workflow_run_test.go

@@ -1176,7 +1176,13 @@ func Test_postWorkflowRunHandler(t *testing.T) {
 	uri := router.GetRoute("POST", api.postWorkflowRunHandler, vars)
 	test.NotEmpty(t, uri)
 
-	opts := &sdk.WorkflowRunPostHandlerOption{}
+	opts := &sdk.WorkflowRunPostHandlerOption{
+		Manual: &sdk.WorkflowNodeRunManual{
+			Payload: map[string]string{
+				"test": "hereismytest",
+			},
+		},
+	}
 	req := assets.NewAuthentifiedRequest(t, u, pass, "POST", uri, opts)
 
 	//Do the request
@@ -1190,6 +1196,22 @@ func Test_postWorkflowRunHandler(t *testing.T) {
 
 	// wait for the workflow to finish crafting
 	assert.NoError(t, waitCraftinWorkflow(t, db, wr.ID))
+
+	lastRun, err := workflow.LoadLastRun(api.mustDB(), proj.Key, w1.Name, workflow.LoadRunOptions{})
+	test.NoError(t, err)
+	assert.NotNil(t, lastRun.RootRun())
+	payloadCount := 0
+	testFound := false
+	for _, param := range lastRun.RootRun().BuildParameters {
+		if param.Name == "payload" {
+			payloadCount++
+		} else if param.Name == "test" {
+			testFound = true
+		}
+	}
+
+	assert.Equal(t, 1, payloadCount)
+	assert.True(t, testFound, "should find 'test' in build parameters")
 }
 
 func waitCraftinWorkflow(t *testing.T, db gorp.SqlExecutor, id int64) error {
@@ -1887,7 +1909,175 @@ func Test_postWorkflowRunHandlerHookWithMutex(t *testing.T) {
 	test.NoError(t, err)
 	assert.Equal(t, int64(2), lastRun.Number)
 	assert.Equal(t, sdk.StatusBuilding, lastRun.Status)
+}
+
+func Test_postWorkflowRunHandlerHook(t *testing.T) {
+	api, db, router, end := newTestAPI(t, bootstrap.InitiliazeDB)
+	defer end()
+	u, pass := assets.InsertAdminUser(t, api.mustDB())
+	key := sdk.RandomString(10)
+	proj := assets.InsertTestProject(t, db, api.Cache, key, key)
+
+	//First pipeline
+	pip := sdk.Pipeline{
+		ProjectID:  proj.ID,
+		ProjectKey: proj.Key,
+		Name:       "pip1",
+	}
+	test.NoError(t, pipeline.InsertPipeline(api.mustDB(), api.Cache, proj, &pip))
+
+	s := sdk.NewStage("stage 1")
+	s.Enabled = true
+	s.PipelineID = pip.ID
+	test.NoError(t, pipeline.InsertStage(api.mustDB(), s))
+	j := &sdk.Job{
+		Enabled: true,
+		Action: sdk.Action{
+			Enabled: true,
+		},
+	}
+	test.NoError(t, pipeline.InsertJob(api.mustDB(), j, s.ID, &pip))
+	s.Jobs = append(s.Jobs, *j)
+
+	pip.Stages = append(pip.Stages, *s)
+
+	//Second pipeline
+	pip2 := sdk.Pipeline{
+		ProjectID:  proj.ID,
+		ProjectKey: proj.Key,
+		Name:       "pip2",
+	}
+	test.NoError(t, pipeline.InsertPipeline(api.mustDB(), api.Cache, proj, &pip2))
+	s = sdk.NewStage("stage 1")
+	s.Enabled = true
+	s.PipelineID = pip2.ID
+	test.NoError(t, pipeline.InsertStage(api.mustDB(), s))
+	j = &sdk.Job{
+		Enabled: true,
+		Action: sdk.Action{
+			Enabled: true,
+		},
+	}
+	test.NoError(t, pipeline.InsertJob(api.mustDB(), j, s.ID, &pip2))
+	s.Jobs = append(s.Jobs, *j)
+
+	mockServiceHook, _ := assets.InsertService(t, db, "Test_postWorkflowRunHandlerHookWithMutex", services.TypeHooks)
+	defer func() {
+		_ = services.Delete(db, mockServiceHook) // nolint
+	}()
+
+	//This is a mock for the hook service
+	services.HTTPClient = mock(
+		func(r *http.Request) (*http.Response, error) {
+			body := new(bytes.Buffer)
+			w := new(http.Response)
+			enc := json.NewEncoder(body)
+			w.Body = ioutil.NopCloser(body)
+
+			switch r.URL.String() {
+			case "/task/bulk":
+				hooks := map[string]sdk.NodeHook{}
+				hooks["1cbf3792-126b-4111-884f-077bdee9523d"] = sdk.NodeHook{
+					HookModelName: sdk.WebHookModel.Name,
+					Config:        sdk.WebHookModel.DefaultConfig.Clone(),
+					UUID:          "1cbf3792-126b-4111-884f-077bdee9523d",
+				}
+				if err := enc.Encode(hooks); err != nil {
+					return writeError(w, err)
+				}
+			default:
+				return writeError(w, fmt.Errorf("route %s must not be called", r.URL.String()))
+			}
+			return w, nil
+		},
+	)
+
+	_, errDb := db.Exec("DELETE FROM w_node_hook WHERE uuid = $1", "1cbf3792-126b-4111-884f-077bdee9523d")
+	test.NoError(t, errDb)
+
+	w := sdk.Workflow{
+		Name:       "test_1",
+		ProjectID:  proj.ID,
+		ProjectKey: proj.Key,
+		HookModels: map[int64]sdk.WorkflowHookModel{
+			1: sdk.WebHookModel,
+		},
+		WorkflowData: &sdk.WorkflowData{
+			Node: sdk.Node{
+				Name: "root",
+				Type: sdk.NodeTypePipeline,
+				Context: &sdk.NodeContext{
+					PipelineID: pip.ID,
+				},
+				Hooks: []sdk.NodeHook{
+					{
+						HookModelName: sdk.WebHookModel.Name,
+						Config:        sdk.WebHookModel.DefaultConfig.Clone(),
+						UUID:          "1cbf3792-126b-4111-884f-077bdee9523d",
+					},
+				},
+			},
+		},
+	}
+
+	proj2, errP := project.Load(api.mustDB(), api.Cache, proj.Key, project.LoadOptions.WithPipelines, project.LoadOptions.WithGroups, project.LoadOptions.WithEnvironments)
+	test.NoError(t, errP)
+
+	test.NoError(t, workflow.Insert(context.TODO(), api.mustDB(), api.Cache, &w, proj2))
+	w1, err := workflow.Load(context.TODO(), api.mustDB(), api.Cache, proj2, "test_1", workflow.LoadOptions{})
+	test.NoError(t, err)
+
+	//Prepare request
+	vars := map[string]string{
+		"key":              proj.Key,
+		"permWorkflowName": w1.Name,
+	}
+	uri := router.GetRoute("POST", api.postWorkflowRunHandler, vars)
+	test.NotEmpty(t, uri)
+
+	opts := &sdk.WorkflowRunPostHandlerOption{
+		Hook: &sdk.WorkflowNodeRunHookEvent{
+			Payload: map[string]string{
+				"test":    "mypayload",
+				"payload": `{"raw": "value"}`,
+			},
+			WorkflowNodeHookUUID: "1cbf3792-126b-4111-884f-077bdee9523d",
+		},
+	}
+	req := assets.NewAuthentifiedRequest(t, u, pass, "POST", uri, opts)
+
+	//Do the request, start first workflow
+	rec := httptest.NewRecorder()
+	router.Mux.ServeHTTP(rec, req)
+	var body []byte
+	_, err = req.Body.Read(body)
+	test.NoError(t, err)
+	defer req.Body.Close()
+	assert.Equal(t, 202, rec.Code)
+	wr := &sdk.WorkflowRun{}
+	require.NoError(t, json.Unmarshal(rec.Body.Bytes(), wr))
+	assert.Equal(t, int64(1), wr.Number)
+
+	assert.NoError(t, waitCraftinWorkflow(t, db, wr.ID))
+	lastRun, err := workflow.LoadLastRun(api.mustDB(), proj.Key, w1.Name, workflow.LoadRunOptions{})
+	test.NoError(t, err)
+	assert.NotNil(t, lastRun.RootRun())
+	payloadCount := 0
+	rawFound := false
+	testFound := false
+	for _, param := range lastRun.RootRun().BuildParameters {
+		if param.Name == "payload" {
+			payloadCount++
+		} else if param.Name == "raw" {
+			rawFound = true
+		} else if param.Name == "test" {
+			testFound = true
+		}
+	}
 
+	assert.Equal(t, 1, payloadCount)
+	assert.False(t, rawFound, "should not find 'raw' in build parameters")
+	assert.True(t, testFound, "should find 'test' in build parameters")
 }
 
 func Test_postWorkflowRunHandler_Forbidden(t *testing.T) {

engine/hooks/outgoing_hooks.go

@@ -213,6 +213,8 @@ func (s *Service) doOutgoingWorkflowExecution(ctx context.Context, t *sdk.TaskEx
 			} else {
 				log.Error(ctx, "Hooks> doOutgoingWorkflowExecution> Cannot unmarshall payload %s", err)
 			}
+
+			payloadValues["payload"] = string(payloadstr)
 		}
 	}
 

engine/hooks/tasks_test.go

@@ -2,6 +2,7 @@ package hooks
 
 import (
 	"context"
+	"net/http"
 	"testing"
 	"time"
 
@@ -39,6 +40,37 @@ func Test_doWebHookExecution(t *testing.T) {
 	assert.Equal(t, "sguiheux", hs[0].Payload["author"])
 	assert.Equal(t, "monmessage", hs[0].Payload["message"])
 	assert.Equal(t, "123456789", hs[0].Payload["hash"])
+	assert.True(t, hs[0].Payload["payload"] != "", "payload should not be empty")
+}
+
+func Test_doWebHookExecutionWithRequestBody(t *testing.T) {
+	log.SetLogger(t)
+	s, cancel := setupTestHookService(t)
+	defer cancel()
+	task := &sdk.TaskExecution{
+		UUID: sdk.RandomString(10),
+		Type: TypeWebHook,
+		WebHook: &sdk.WebHookExecution{
+			RequestMethod: string(http.MethodPost),
+			RequestHeader: map[string][]string{
+				"Content-Type": []string{
+					"application/json",
+				},
+			},
+			RequestBody: []byte(`{"test": "hereisatest"}`),
+		},
+		Config: sdk.WorkflowNodeHookConfig{
+			"method": sdk.WorkflowNodeHookConfigValue{
+				Value: string(http.MethodPost),
+			},
+		},
+	}
+	hs, err := s.doWebHookExecution(context.TODO(), task)
+	test.NoError(t, err)
+
+	assert.Equal(t, 1, len(hs))
+	assert.Equal(t, "hereisatest", hs[0].Payload["test"])
+	assert.True(t, hs[0].Payload["payload"] != "", "payload should not be empty")
 }
 
 func Test_dequeueTaskExecutions_ScheduledTask(t *testing.T) {

engine/hooks/webhook.go

@@ -141,9 +141,10 @@ func executeWebHook(t *sdk.TaskExecution) (*sdk.WorkflowNodeRunHookEvent, error)
 		case ct == "application/x-www-form-urlencoded":
 			formValues, err := url.ParseQuery(string(t.WebHook.RequestBody))
 			if err == nil {
-				return nil, sdk.WrapError(err, "Unable webhookto parse body %s", t.WebHook.RequestBody)
+				return nil, sdk.WrapError(err, "Unable webhook to parse body %s", t.WebHook.RequestBody)
 			}
 			copyValues(values, formValues)
+			h.Payload["payload"] = string(t.WebHook.RequestBody)
 		case ct == "application/json":
 			var bodyJSON interface{}
 
@@ -177,6 +178,12 @@ func executeWebHook(t *sdk.TaskExecution) (*sdk.WorkflowNodeRunHookEvent, error)
 				values.Add(k, v)
 			}
 			h.Payload["payload"] = string(t.WebHook.RequestBody)
+		default:
+			h.Payload["payload"] = t.WebHook.RequestURL
+		}
+	} else {
+		if _, ok := h.Payload["payload"]; !ok {
+			h.Payload["payload"] = t.WebHook.RequestURL
 		}
 	}