chore: minor refactor of webhook module

pkg/modules/webhook/middleware_test.go

@@ -45,8 +45,8 @@ func TestWebhookMiddlewareGuards(t *testing.T) {
 		return req
 	}
 
-	buildWebhookModule := func() Webhook {
-		return Webhook{
+	buildWebhookModule := func() *Webhook {
+		return &Webhook{
 			allowList:      regexp.MustCompile(""),
 			denyList:       regexp.MustCompile(""),
 			errorAllowList: regexp.MustCompile(""),
@@ -58,238 +58,240 @@ func TestWebhookMiddlewareGuards(t *testing.T) {
 		}
 	}
 
-	for i, tc := range []struct {
+	for _, tc := range []struct {
+		scenario         string
 		request          *http.Request
-		mod              Webhook
+		mod              *Webhook
 		next             echo.HandlerFunc
-		expectErr        bool
-		expectHTTPErr    bool
-		expectHTTPStatus int
+		expectError      bool
+		expectHttpError  bool
+		expectHttpStatus int
 	}{
 		{
-			request: buildMultipartFormDataRequest(),
-			mod:     buildWebhookModule(),
+			scenario: "no webhook URL, skip middleware",
+			request:  buildMultipartFormDataRequest(),
+			mod:      buildWebhookModule(),
 			next: func() echo.HandlerFunc {
 				return func(c echo.Context) error {
 					return nil
 				}
 			}(),
+			expectError:     false,
+			expectHttpError: false,
 		},
 		{
+			scenario: "no webhook error URL",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
-
 				return req
 			}(),
 			mod:              buildWebhookModule(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusBadRequest,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusBadRequest,
 		},
 		{
+			scenario: "webhook URL is not allowed",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
-
 				return req
 			}(),
-			mod: func() Webhook {
+			mod: func() *Webhook {
 				mod := buildWebhookModule()
 				mod.allowList = regexp.MustCompile("bar")
-
 				return mod
 			}(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusForbidden,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusForbidden,
 		},
 		{
+			scenario: "webhook URL is denied",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
-
 				return req
 			}(),
-			mod: func() Webhook {
+			mod: func() *Webhook {
 				mod := buildWebhookModule()
 				mod.denyList = regexp.MustCompile("foo")
-
 				return mod
 			}(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusForbidden,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusForbidden,
 		},
 		{
+			scenario: "webhook error URL is not allowed",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
-
 				return req
 			}(),
-			mod: func() Webhook {
+			mod: func() *Webhook {
 				mod := buildWebhookModule()
 				mod.errorAllowList = regexp.MustCompile("foo")
-
 				return mod
 			}(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusForbidden,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusForbidden,
 		},
 		{
+			scenario: "webhook error URL is denied",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
-
 				return req
 			}(),
-			mod: func() Webhook {
+			mod: func() *Webhook {
 				mod := buildWebhookModule()
 				mod.errorDenyList = regexp.MustCompile("bar")
-
 				return mod
 			}(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusForbidden,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusForbidden,
 		},
 		{
+			scenario: "invalid webhook method (GET)",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Method", http.MethodGet)
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
-
 				return req
 			}(),
 			mod:              buildWebhookModule(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusBadRequest,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusBadRequest,
 		},
 		{
+			scenario: "invalid webhook error method (GET)",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
 				req.Header.Set("Gotenberg-Webhook-Error-Method", http.MethodGet)
-
 				return req
 			}(),
 			mod:              buildWebhookModule(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusBadRequest,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusBadRequest,
 		},
 		{
+			scenario: "valid webhook method (POST) but invalid webhook error method (GET)",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Method", http.MethodPost)
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
 				req.Header.Set("Gotenberg-Webhook-Error-Method", http.MethodGet)
-
 				return req
 			}(),
 			mod:              buildWebhookModule(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusBadRequest,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusBadRequest,
 		},
 		{
+			scenario: "valid webhook method (PATH) but invalid webhook error method (GET)",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Method", http.MethodPatch)
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
 				req.Header.Set("Gotenberg-Webhook-Error-Method", http.MethodGet)
-
 				return req
 			}(),
 			mod:              buildWebhookModule(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusBadRequest,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusBadRequest,
 		},
 		{
+			scenario: "valid webhook method (PUT) but invalid webhook error method (GET)",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Method", http.MethodPut)
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
 				req.Header.Set("Gotenberg-Webhook-Error-Method", http.MethodGet)
-
 				return req
 			}(),
 			mod:              buildWebhookModule(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusBadRequest,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusBadRequest,
 		},
 		{
+			scenario: "invalid webhook extra HTTP headers",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Webhook-Url", "foo")
 				req.Header.Set("Gotenberg-Webhook-Error-Url", "bar")
 				req.Header.Set("Gotenberg-Webhook-Extra-Http-Headers", "foo")
-
 				return req
 			}(),
 			mod:              buildWebhookModule(),
-			expectErr:        true,
-			expectHTTPErr:    true,
-			expectHTTPStatus: http.StatusBadRequest,
+			expectError:      true,
+			expectHttpError:  true,
+			expectHttpStatus: http.StatusBadRequest,
 		},
 	} {
-		srv := echo.New()
-		srv.HideBanner = true
-		srv.HidePort = true
+		t.Run(tc.scenario, func(t *testing.T) {
+			srv := echo.New()
+			srv.HideBanner = true
+			srv.HidePort = true
 
-		c := srv.NewContext(tc.request, httptest.NewRecorder())
+			c := srv.NewContext(tc.request, httptest.NewRecorder())
 
-		ctx := &api.ContextMock{Context: &api.Context{}}
-		ctx.SetEchoContext(c)
+			ctx := &api.ContextMock{Context: &api.Context{}}
+			ctx.SetEchoContext(c)
 
-		c.Set("context", ctx.Context)
-		c.Set("cancel", func() context.CancelFunc {
-			return func() {
-				return
+			c.Set("context", ctx.Context)
+			c.Set("cancel", func() context.CancelFunc {
+				return func() {
+					return
+				}
+			}())
+
+			err := webhookMiddleware(tc.mod).Handler(tc.next)(c)
+
+			if tc.expectError && err == nil {
+				t.Fatal("expected error but got none", err)
 			}
-		}())
 
-		err := webhookMiddleware(tc.mod).Handler(tc.next)(c)
-
-		if tc.expectErr && err == nil {
-			t.Errorf("test %d: expected error but got: %v", i, err)
-		}
-
-		if !tc.expectErr && err != nil {
-			t.Errorf("test %d: expected no error but got: %v", i, err)
-		}
-
-		var httpErr api.HTTPError
-		isHTTPErr := errors.As(err, &httpErr)
-
-		if tc.expectHTTPErr && !isHTTPErr {
-			t.Errorf("test %d: expected HTTP error but got: %v", i, err)
-		}
-
-		if !tc.expectHTTPErr && isHTTPErr {
-			t.Errorf("test %d: expected no HTTP error but got one: %v", i, httpErr)
-		}
-
-		if err != nil && tc.expectHTTPErr && isHTTPErr {
-			status, _ := httpErr.HTTPError()
-			if status != tc.expectHTTPStatus {
-				t.Errorf("test %d: expected %d HTTP status code but got %d", i, tc.expectHTTPStatus, status)
+			if !tc.expectError && err != nil {
+				t.Fatalf("expected no error but got: %v", err)
 			}
-		}
+
+			var httpErr api.HTTPError
+			isHTTPErr := errors.As(err, &httpErr)
+
+			if tc.expectHttpError && !isHTTPErr {
+				t.Errorf("expected an HTTP error but got: %v", err)
+			}
+
+			if !tc.expectHttpError && isHTTPErr {
+				t.Errorf("expected no HTTP error but got one: %v", httpErr)
+			}
+
+			if err != nil && tc.expectHttpError && isHTTPErr {
+				status, _ := httpErr.HTTPError()
+				if status != tc.expectHttpStatus {
+					t.Errorf("expected %d as HTTP status code but got %d", tc.expectHttpStatus, status)
+				}
+			}
+		})
 	}
 }
 
@@ -316,8 +318,8 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 		return req
 	}
 
-	buildWebhookModule := func() Webhook {
-		return Webhook{
+	buildWebhookModule := func() *Webhook {
+		return &Webhook{
 			allowList:      regexp.MustCompile(""),
 			denyList:       regexp.MustCompile(""),
 			errorAllowList: regexp.MustCompile(""),
@@ -330,21 +332,23 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 		}
 	}
 
-	for i, tc := range []struct {
+	for _, tc := range []struct {
+		scenario                      string
 		request                       *http.Request
-		mod                           Webhook
+		mod                           *Webhook
 		next                          echo.HandlerFunc
 		expectWebhookContentType      string
 		expectWebhookMethod           string
-		expectWebhookExtraHTTPHeaders map[string]string
+		expectWebhookExtraHttpHeaders map[string]string
 		expectWebhookFilename         string
 		expectWebhookErrorStatus      int
 		expectWebhookErrorMessage     string
 		returnedError                 *echo.HTTPError
 	}{
 		{
-			request: buildMultipartFormDataRequest(),
-			mod:     buildWebhookModule(),
+			scenario: "next handler return an error",
+			request:  buildMultipartFormDataRequest(),
+			mod:      buildWebhookModule(),
 			next: func() echo.HandlerFunc {
 				return func(c echo.Context) error {
 					return errors.New("foo")
@@ -356,8 +360,9 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 			expectWebhookErrorMessage: http.StatusText(http.StatusInternalServerError),
 		},
 		{
-			request: buildMultipartFormDataRequest(),
-			mod:     buildWebhookModule(),
+			scenario: "next handler return an HTTP error",
+			request:  buildMultipartFormDataRequest(),
+			mod:      buildWebhookModule(),
 			next: func() echo.HandlerFunc {
 				return func(c echo.Context) error {
 					return api.NewSentinelHTTPError(http.StatusBadRequest, http.StatusText(http.StatusBadRequest))
@@ -369,38 +374,36 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 			expectWebhookErrorMessage: http.StatusText(http.StatusBadRequest),
 		},
 		{
+			scenario: "success",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Output-Filename", "foo")
 				req.Header.Set("Gotenberg-Webhook-Extra-Http-Headers", `{ "foo": "bar" }`)
-
 				return req
 			}(),
 			mod: buildWebhookModule(),
 			next: func() echo.HandlerFunc {
 				return func(c echo.Context) error {
 					ctx := c.Get("context").(*api.Context)
-
 					return ctx.AddOutputPaths("/tests/test/testdata/api/sample2.pdf")
 				}
 			}(),
 			expectWebhookContentType:      "application/pdf",
 			expectWebhookMethod:           http.MethodPost,
 			expectWebhookFilename:         "foo",
-			expectWebhookExtraHTTPHeaders: map[string]string{"foo": "bar"},
+			expectWebhookExtraHttpHeaders: map[string]string{"foo": "bar"},
 		},
 		{
+			scenario: "success (return an error)",
 			request: func() *http.Request {
 				req := buildMultipartFormDataRequest()
 				req.Header.Set("Gotenberg-Output-Filename", "foo")
-
 				return req
 			}(),
 			mod: buildWebhookModule(),
 			next: func() echo.HandlerFunc {
 				return func(c echo.Context) error {
 					ctx := c.Get("context").(*api.Context)
-
 					return ctx.AddOutputPaths("/tests/test/testdata/api/sample1.pdf")
 				}
 			}(),
@@ -452,24 +455,24 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 					return func(c echo.Context) error {
 						contentType := c.Request().Header.Get(echo.HeaderContentType)
 						if contentType != tc.expectWebhookContentType {
-							t.Errorf("test %d: expected '%s' '%s' but got '%s'", i, echo.HeaderContentType, tc.expectWebhookContentType, contentType)
+							t.Errorf("expected '%s' '%s' but got '%s'", echo.HeaderContentType, tc.expectWebhookContentType, contentType)
 						}
 
 						trace := c.Request().Header.Get("Gotenberg-Trace")
 						if trace != "foo" {
-							t.Errorf("test %d: expected '%s' '%s' but got '%s'", i, "Gotenberg-Trace", "foo", trace)
+							t.Errorf("expected '%s' '%s' but got '%s'", "Gotenberg-Trace", "foo", trace)
 						}
 
 						method := c.Request().Method
 						if method != tc.expectWebhookMethod {
-							t.Errorf("test %d: expected HTTP method '%s' but got '%s'", i, tc.expectWebhookMethod, method)
+							t.Errorf("expected HTTP method '%s' but got '%s'", tc.expectWebhookMethod, method)
 						}
 
-						for key, expect := range tc.expectWebhookExtraHTTPHeaders {
+						for key, expect := range tc.expectWebhookExtraHttpHeaders {
 							actual := c.Request().Header.Get(key)
 
 							if actual != expect {
-								t.Errorf("test %d: expected '%s' '%s' but got '%s'", i, key, expect, actual)
+								t.Errorf("expected '%s' '%s' but got '%s'", key, expect, actual)
 							}
 						}
 
@@ -492,11 +495,11 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 							}
 
 							if result.Status != tc.expectWebhookErrorStatus {
-								t.Errorf("test %d: expected status %d from JSON but got %d", i, tc.expectWebhookErrorStatus, result.Status)
+								t.Errorf("expected status %d from JSON but got %d", tc.expectWebhookErrorStatus, result.Status)
 							}
 
 							if result.Message != tc.expectWebhookErrorMessage {
-								t.Errorf("test %d: expected message '%s' from JSON but got '%s'", i, tc.expectWebhookErrorMessage, result.Message)
+								t.Errorf("expected message '%s' from JSON but got '%s'", tc.expectWebhookErrorMessage, result.Message)
 							}
 
 							errChan <- nil
@@ -505,12 +508,12 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 
 						contentLength := c.Request().Header.Get(echo.HeaderContentLength)
 						if contentLength == "" {
-							t.Errorf("test %d: expected non empty '%s'", i, echo.HeaderContentLength)
+							t.Errorf("expected non empty '%s'", echo.HeaderContentLength)
 						}
 
 						contentDisposition := c.Request().Header.Get(echo.HeaderContentDisposition)
 						if !strings.Contains(contentDisposition, tc.expectWebhookFilename) {
-							t.Errorf("test %d: expected '%s' '%s' to contain '%s'", i, echo.HeaderContentDisposition, contentDisposition, tc.expectWebhookFilename)
+							t.Errorf("expected '%s' '%s' to contain '%s'", echo.HeaderContentDisposition, contentDisposition, tc.expectWebhookFilename)
 						}
 
 						body, err := io.ReadAll(c.Request().Body)
@@ -520,7 +523,7 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 						}
 
 						if body == nil || len(body) == 0 {
-							t.Errorf("test %d: expected non nil body", i)
+							t.Error("expected non nil body")
 						}
 
 						errChan <- nil
@@ -537,25 +540,25 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) {
 			go func() {
 				err := webhook.Start(fmt.Sprintf(":%d", webhookPort))
 				if !errors.Is(err, http.ErrServerClosed) {
-					t.Errorf("test %d: expected no error but got: %v", i, err)
+					t.Errorf("expected no error but got: %v", err)
 				}
 			}()
 
 			defer func() {
 				err := webhook.Shutdown(context.TODO())
 				if err != nil {
-					t.Errorf("test %d: expected no error but got: %v", i, err)
+					t.Errorf("expected no error but got: %v", err)
 				}
 			}()
 
 			err := webhookMiddleware(tc.mod).Handler(tc.next)(c)
 			if err != nil && !errors.Is(err, api.ErrAsyncProcess) {
-				t.Errorf("test %d: expected no error but got: %v", i, err)
+				t.Errorf("expected no error but got: %v", err)
 			}
 
 			err = <-errChan
 			if err != nil {
-				t.Errorf("test %d: expected no error but got: %v", i, err)
+				t.Errorf("expected no error but got: %v", err)
 			}
 		}()
 	}