From 51fce88d24af0a3a7d10cfa49174703823b54adc Mon Sep 17 00:00:00 2001
From: shydefoo <shidefoo@gmail.com>
Date: Tue, 6 Aug 2024 09:24:05 +0800
Subject: [PATCH] Minor fixes for webhooks package & project creation (#105)

* Add new errors file in webhooks package

* Webhook errors are now wrapped as WebhookErrors
* This is for better error handling

* Set numRetries to default to 3

* Set retry error to last error only
---
 api/api/projects_api.go         | 13 +++++++++++++
 api/pkg/webhooks/client.go      |  9 ++++++---
 api/pkg/webhooks/errors.go      | 29 +++++++++++++++++++++++++++++
 api/service/projects_service.go | 27 +++++++++++++++++----------
 4 files changed, 65 insertions(+), 13 deletions(-)
 create mode 100644 api/pkg/webhooks/errors.go

diff --git a/api/api/projects_api.go b/api/api/projects_api.go
index a2998a0c..1a0ce1cb 100644
--- a/api/api/projects_api.go
+++ b/api/api/projects_api.go
@@ -9,6 +9,7 @@ import (
 	"github.com/caraml-dev/mlp/api/log"
 	"github.com/caraml-dev/mlp/api/models"
 	apperror "github.com/caraml-dev/mlp/api/pkg/errors"
+	"github.com/caraml-dev/mlp/api/pkg/webhooks"
 )
 
 type ProjectsController struct {
@@ -51,7 +52,19 @@ func (c *ProjectsController) CreateProject(r *http.Request, vars map[string]stri
 	user := vars["user"]
 	project.Administrators = addRequester(user, project.Administrators)
 	project, err = c.ProjectsService.CreateProject(r.Context(), project)
+	var webhookError *webhooks.WebhookError
 	if err != nil {
+		// NOTE: Here we are checking if the error is a WebhookError
+		// This is to improve the error message shared with the user,
+		// since the current logic creates the Project first before firing
+		// the ProjectCreatedEvent webhook.
+		if errors.As(err, &webhookError) {
+			errMsg := fmt.Errorf(`Project %s was created, 
+			but not all webhooks were correctly invoked. 
+			Some additional resources may not have been created successfully`, project.Name)
+			log.Errorf("%s, %s", errMsg, err)
+			return FromError(errMsg)
+		}
 		log.Errorf("error creating project %s: %s", project.Name, err)
 		return FromError(err)
 	}
diff --git a/api/pkg/webhooks/client.go b/api/pkg/webhooks/client.go
index 35e0d454..e4627947 100644
--- a/api/pkg/webhooks/client.go
+++ b/api/pkg/webhooks/client.go
@@ -103,14 +103,14 @@ func (g *simpleWebhookClient) Invoke(ctx context.Context, payload []byte) ([]byt
 			}
 			// check http status code
 			if resp.StatusCode != http.StatusOK {
-				return fmt.Errorf("response status code %d not 200", resp.StatusCode)
+				return fmt.Errorf("response status code %d not 200, err: %s", resp.StatusCode, content)
 			}
 			return nil
 
-		}, retry.Attempts(uint(g.NumRetries)), retry.Context(ctx),
+		}, retry.Attempts(uint(g.NumRetries)), retry.Context(ctx), retry.LastErrorOnly(true),
 	)
 	if err != nil {
-		return nil, err
+		return nil, NewWebhookError(err)
 	}
 	return content, nil
 }
@@ -152,4 +152,7 @@ func setDefaults(webhookConfig *WebhookConfig) {
 		def := 10
 		webhookConfig.Timeout = &def
 	}
+	if webhookConfig.NumRetries == 0 {
+		webhookConfig.NumRetries = 3
+	}
 }
diff --git a/api/pkg/webhooks/errors.go b/api/pkg/webhooks/errors.go
new file mode 100644
index 00000000..f8287f0c
--- /dev/null
+++ b/api/pkg/webhooks/errors.go
@@ -0,0 +1,29 @@
+package webhooks
+
+import "fmt"
+
+type WebhookError struct {
+	msg string
+	err error
+}
+
+func NewWebhookError(err error) *WebhookError {
+	return &WebhookError{
+		msg: "error invoking webhook",
+		err: err,
+	}
+}
+
+// Implement errors.Error method
+func (e *WebhookError) Error() string {
+	return fmt.Sprintf("%s: %v", e.msg, e.err)
+}
+
+func (e *WebhookError) Unwrap() error {
+	return e.err
+}
+
+func (e *WebhookError) Is(target error) bool {
+	_, ok := target.(*WebhookError)
+	return ok
+}
diff --git a/api/service/projects_service.go b/api/service/projects_service.go
index 5f366155..df8fc586 100644
--- a/api/service/projects_service.go
+++ b/api/service/projects_service.go
@@ -17,6 +17,7 @@ import (
 	"github.com/caraml-dev/mlp/api/repository"
 
 	"github.com/caraml-dev/mlp/api/config"
+	"github.com/caraml-dev/mlp/api/log"
 	"github.com/caraml-dev/mlp/api/models"
 	"github.com/caraml-dev/mlp/api/pkg/authz/enforcer"
 	"github.com/caraml-dev/mlp/api/pkg/webhooks"
@@ -107,18 +108,21 @@ func (service *projectsService) CreateProject(ctx context.Context, project *mode
 		// Expects webhook output to be a project object
 		var tmpproject models.Project
 		if err := json.Unmarshal(p, &tmpproject); err != nil {
-			return err
+			return webhooks.NewWebhookError(err)
 		}
 		project, err = service.save(&tmpproject)
 		if err != nil {
-			return err
+			return webhooks.NewWebhookError(err)
 		}
 		return nil
-	}, webhooks.NoOpErrorHandler)
+	}, func(err error) error {
+		// Print error and return
+		log.Errorf("error calling webhook - %s, err: %s", ProjectCreatedEvent, err.Error())
+		return err
+	},
+	)
 	if err != nil {
-		return project,
-			fmt.Errorf("error while invoking %s webhooks or on success callback function, err: %s",
-				ProjectCreatedEvent, err.Error())
+		return project, err
 	}
 	return project, nil
 }
@@ -165,11 +169,14 @@ func (service *projectsService) UpdateProject(ctx context.Context, project *mode
 				return err
 			}
 			return nil
-		}, webhooks.NoOpErrorHandler)
+		}, func(err error) error {
+			// Print error and return
+			log.Errorf("error calling webhook - %s, err: %s", ProjectCreatedEvent, err.Error())
+			return err
+		},
+		)
 		if err != nil {
-			return project, nil,
-				fmt.Errorf("error while invoking %s webhooks or on success callback function, err: %s",
-					ProjectUpdatedEvent, err.Error())
+			return project, nil, err
 		}
 	} else {
 		project, err = service.save(project)