Merge pull request #980 from kohnalex/#979_fix_fibersentry_interface_…

…conversion

fix(#979): GetHubFromContext may crash instead of returning nil

fibersentry/README.md

@@ -33,18 +33,18 @@ fibersentry.New(config ...fibersentry.Config) fiber.Handler
 ## Config
 
 | Property        | Type            | Description                                                                                                                                                                                                                                                          | Default           |
-|:----------------|:----------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:------------------|
+| :-------------- | :-------------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :---------------- |
 | Repanic         | `bool`          | Repanic configures whether Sentry should repanic after recovery. Set to true, if [Recover](https://github.com/gofiber/fiber/tree/master/middleware/recover) middleware is used.                                                                                      | `false`           |
 | WaitForDelivery | `bool`          | WaitForDelivery configures whether you want to block the request before moving forward with the response. If [Recover](https://github.com/gofiber/fiber/tree/master/middleware/recover) middleware is used, it's safe to either skip this option or set it to false. | `false`           |
 | Timeout         | `time.Duration` | Timeout for the event delivery requests.                                                                                                                                                                                                                             | `time.Second * 2` |
 
 ## Usage
 
 `fibersentry` attaches an instance of `*sentry.Hub` (https://godoc.org/github.com/getsentry/sentry-go#Hub) to the request's context, which makes it available throughout the rest of the request's lifetime.
-You can access it by using the `fibersentry.GetHubFromContext()` method on the context itself in any of your proceeding middleware and routes.
-And it should be used instead of the global `sentry.CaptureMessage`, `sentry.CaptureException`, or any other calls, as it keeps the separation of data between the requests.
+You can access it by using the `fibersentry.GetHubFromContext()` or `fibersentry.MustGetHubFromContext()` method on the context itself in any of your proceeding middleware and routes.
+Keep in mind that `*sentry.Hub` should be used instead of the global `sentry.CaptureMessage`, `sentry.CaptureException`, or any other calls, as it keeps the separation of data between the requests.
 
-**Keep in mind that `*sentry.Hub` won't be available in middleware attached before to `fibersentry`!**
+- **Keep in mind that `*sentry.Hub` won't be available in middleware attached before `fibersentry`. In this case, `GetHubFromContext()` returns nil, and `MustGetHubFromContext()` will panic.**
 
 ```go
 package main

fibersentry/sentry.go

@@ -18,7 +18,6 @@ func New(config ...Config) fiber.Handler {
 	return func(c *fiber.Ctx) error {
 		// Convert fiber request to http request
 		r, err := adaptor.ConvertRequest(c, true)
-
 		if err != nil {
 			return err
 		}
@@ -53,6 +52,14 @@ func New(config ...Config) fiber.Handler {
 	}
 }
 
-func GetHubFromContext(ctx *fiber.Ctx) *sentry.Hub {
+func MustGetHubFromContext(ctx *fiber.Ctx) *sentry.Hub {
 	return ctx.Locals(hubKey).(*sentry.Hub)
 }
+
+func GetHubFromContext(ctx *fiber.Ctx) *sentry.Hub {
+	hub, ok := ctx.Locals(hubKey).(*sentry.Hub)
+	if !ok {
+		return nil
+	}
+	return hub
+}

fibersentry/sentry_test.go

@@ -12,141 +12,175 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func Test_Sentry(t *testing.T) {
-	app := fiber.New()
-	app.Use(New())
+type testCase struct {
+	desc    string
+	path    string
+	method  string
+	body    string
+	handler fiber.Handler
+	event   *sentry.Event
+}
 
-	testCases := []struct {
-		desc    string
-		path    string
-		method  string
-		body    string
-		handler fiber.Handler
-		event   *sentry.Event
-	}{
+func testCasesBeforeRegister(t *testing.T) []testCase {
+	return []testCase{
 		{
-			desc:   "panic",
-			path:   "/panic",
+			desc:   "MustGetHubFromContext without Sentry middleware",
+			path:   "/no-middleware",
 			method: "GET",
 			handler: func(c *fiber.Ctx) error {
-				panic("test")
-			},
-			event: &sentry.Event{
-				Level:   sentry.LevelFatal,
-				Message: "test",
-				Request: &sentry.Request{
-					URL:    "http://example.com/panic",
-					Method: "GET",
-					Headers: map[string]string{
-						"Host":       "example.com",
-						"User-Agent": "fiber",
-					},
-				},
-			},
-		},
-		{
-			desc:   "post",
-			path:   "/post",
-			method: "POST",
-			body:   "payload",
-			handler: func(c *fiber.Ctx) error {
-				hub := GetHubFromContext(c)
-				hub.CaptureMessage("post: " + string(c.Body()))
+				defer func() {
+					if r := recover(); r == nil {
+						t.Fatal("MustGetHubFromContext did not panic")
+					}
+				}()
+				_ = MustGetHubFromContext(c) // This should panic
 				return nil
 			},
-			event: &sentry.Event{
-				Level:   sentry.LevelInfo,
-				Message: "post: payload",
-				Request: &sentry.Request{
-					URL:    "http://example.com/post",
-					Method: "POST",
-					Data:   "payload",
-					Headers: map[string]string{
-						"Content-Length": "7",
-						"Host":           "example.com",
-						"User-Agent":     "fiber",
-					},
-				},
-			},
+			event: nil, // No event expected because a panic should occur
 		},
 		{
-			desc:   "get",
-			path:   "/get",
+			desc:   "GetHubFromContext without Sentry middleware",
+			path:   "/no-middleware-2",
 			method: "GET",
 			handler: func(c *fiber.Ctx) error {
 				hub := GetHubFromContext(c)
-				hub.CaptureMessage("get")
+				if hub != nil {
+					t.Fatal("Expected nil, got a Sentry hub instance")
+				}
 				return nil
 			},
-			event: &sentry.Event{
-				Level:   sentry.LevelInfo,
-				Message: "get",
-				Request: &sentry.Request{
-					URL:    "http://example.com/get",
-					Method: "GET",
-					Headers: map[string]string{
-						"Host":       "example.com",
-						"User-Agent": "fiber",
-					},
+			event: nil, // No Sentry event expected here
+		},
+	}
+}
+
+var testCasesAfterRegister = []testCase{
+	{
+		desc:   "panic",
+		path:   "/panic",
+		method: "GET",
+		handler: func(c *fiber.Ctx) error {
+			panic("test")
+		},
+		event: &sentry.Event{
+			Level:   sentry.LevelFatal,
+			Message: "test",
+			Request: &sentry.Request{
+				URL:    "http://example.com/panic",
+				Method: "GET",
+				Headers: map[string]string{
+					"Host":       "example.com",
+					"User-Agent": "fiber",
 				},
 			},
 		},
-		{
-			desc:   "large body",
-			path:   "/post/large",
-			method: "POST",
-			body:   strings.Repeat("Large", 3*1024), // 15 KB
-			handler: func(c *fiber.Ctx) error {
-				hub := GetHubFromContext(c)
-				hub.CaptureMessage(fmt.Sprintf("post: %d KB", len(c.Body())/1024))
-				return nil
+	},
+	{
+		desc:   "post",
+		path:   "/post",
+		method: "POST",
+		body:   "payload",
+		handler: func(c *fiber.Ctx) error {
+			hub := MustGetHubFromContext(c)
+			hub.CaptureMessage("post: " + string(c.Body()))
+			return nil
+		},
+		event: &sentry.Event{
+			Level:   sentry.LevelInfo,
+			Message: "post: payload",
+			Request: &sentry.Request{
+				URL:    "http://example.com/post",
+				Method: "POST",
+				Data:   "payload",
+				Headers: map[string]string{
+					"Content-Length": "7",
+					"Host":           "example.com",
+					"User-Agent":     "fiber",
+				},
 			},
-			event: &sentry.Event{
-				Level:   sentry.LevelInfo,
-				Message: "post: 15 KB",
-				Request: &sentry.Request{
-					URL:    "http://example.com/post/large",
-					Method: "POST",
-					// Actual request body omitted because too large.
-					Data: "",
-					Headers: map[string]string{
-						"Content-Length": "15360",
-						"Host":           "example.com",
-						"User-Agent":     "fiber",
-					},
+		},
+	},
+	{
+		desc:   "get",
+		path:   "/get",
+		method: "GET",
+		handler: func(c *fiber.Ctx) error {
+			hub := MustGetHubFromContext(c)
+			hub.CaptureMessage("get")
+			return nil
+		},
+		event: &sentry.Event{
+			Level:   sentry.LevelInfo,
+			Message: "get",
+			Request: &sentry.Request{
+				URL:    "http://example.com/get",
+				Method: "GET",
+				Headers: map[string]string{
+					"Host":       "example.com",
+					"User-Agent": "fiber",
 				},
 			},
 		},
-		{
-			desc:   "ignore body",
-			path:   "/post/body-ignored",
-			method: "POST",
-			body:   "client sends, fasthttp always reads, SDK reports",
-			handler: func(c *fiber.Ctx) error {
-				hub := GetHubFromContext(c)
-				hub.CaptureMessage("body ignored")
-				return nil
+	},
+	{
+		desc:   "large body",
+		path:   "/post/large",
+		method: "POST",
+		body:   strings.Repeat("Large", 3*1024), // 15 KB
+		handler: func(c *fiber.Ctx) error {
+			hub := MustGetHubFromContext(c)
+			hub.CaptureMessage(fmt.Sprintf("post: %d KB", len(c.Body())/1024))
+			return nil
+		},
+		event: &sentry.Event{
+			Level:   sentry.LevelInfo,
+			Message: "post: 15 KB",
+			Request: &sentry.Request{
+				URL:    "http://example.com/post/large",
+				Method: "POST",
+				// Actual request body omitted because too large.
+				Data: "",
+				Headers: map[string]string{
+					"Content-Length": "15360",
+					"Host":           "example.com",
+					"User-Agent":     "fiber",
+				},
 			},
-			event: &sentry.Event{
-				Level:   sentry.LevelInfo,
-				Message: "body ignored",
-				Request: &sentry.Request{
-					URL:    "http://example.com/post/body-ignored",
-					Method: "POST",
-					// Actual request body included because fasthttp always
-					// reads full request body.
-					Data: "client sends, fasthttp always reads, SDK reports",
-					Headers: map[string]string{
-						"Content-Length": "48",
-						"Host":           "example.com",
-						"User-Agent":     "fiber",
-					},
+		},
+	},
+	{
+		desc:   "ignore body",
+		path:   "/post/body-ignored",
+		method: "POST",
+		body:   "client sends, fasthttp always reads, SDK reports",
+		handler: func(c *fiber.Ctx) error {
+			hub := MustGetHubFromContext(c)
+			hub.CaptureMessage("body ignored")
+			return nil
+		},
+		event: &sentry.Event{
+			Level:   sentry.LevelInfo,
+			Message: "body ignored",
+			Request: &sentry.Request{
+				URL:    "http://example.com/post/body-ignored",
+				Method: "POST",
+				// Actual request body included because fasthttp always
+				// reads full request body.
+				Data: "client sends, fasthttp always reads, SDK reports",
+				Headers: map[string]string{
+					"Content-Length": "48",
+					"Host":           "example.com",
+					"User-Agent":     "fiber",
 				},
 			},
 		},
-	}
+	},
+}
+
+func Test_Sentry(t *testing.T) {
+	app := fiber.New()
 
-	for _, tC := range testCases {
+	testFunc := func(t *testing.T, tC testCase) {
 		t.Run(tC.desc, func(t *testing.T) {
 			if err := sentry.Init(sentry.ClientOptions{
 				BeforeSend: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event {
@@ -180,6 +214,16 @@ func Test_Sentry(t *testing.T) {
 		})
 	}
 
+	for _, tC := range testCasesBeforeRegister(t) {
+		testFunc(t, tC)
+	}
+
+	app.Use(New())
+
+	for _, tC := range testCasesAfterRegister {
+		testFunc(t, tC)
+	}
+
 	if ok := sentry.Flush(time.Second); !ok {
 		t.Fatal("sentry.Flush timed out")
 	}