From b04e2a04cd01d022319d5e3f6e9707b4abc88b49 Mon Sep 17 00:00:00 2001 From: Piti Ongmongkolkul Date: Fri, 30 Dec 2022 17:00:50 +0700 Subject: [PATCH 1/3] fix: json/xml now has proper trace field when using development or test --- default_context.go | 3 ++- errors.go | 36 +++++++++++++++++++++++++++++++++--- errors_test.go | 21 +++++++++++++++++---- go.mod | 1 + go.sum | 2 ++ 5 files changed, 55 insertions(+), 8 deletions(-) diff --git a/default_context.go b/default_context.go index e88acbf4b..e3c9d110b 100644 --- a/default_context.go +++ b/default_context.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + pkgError "github.com/pkg/errors" "io" "net/http" "net/url" @@ -196,7 +197,7 @@ func (d *DefaultContext) LogFields(values map[string]interface{}) { } func (d *DefaultContext) Error(status int, err error) error { - return HTTPError{Status: status, Cause: err} + return HTTPError{Status: status, Cause: pkgError.WithStack(err)} } var mapType = reflect.ValueOf(map[string]interface{}{}).Type() diff --git a/errors.go b/errors.go index 04ff0fef7..89af5e638 100644 --- a/errors.go +++ b/errors.go @@ -15,6 +15,7 @@ import ( "github.com/gobuffalo/buffalo/internal/httpx" "github.com/gobuffalo/events" "github.com/gobuffalo/plush/v4" + pkgError "github.com/pkg/errors" ) // HTTPError a typed error returned by http Handlers and used for choosing error handlers @@ -36,6 +37,16 @@ func (h HTTPError) Error() string { return "unknown cause" } +func (h HTTPError) StackTrace() pkgError.StackTrace { + if h.Cause == nil { + return nil + } + if tracer, ok := h.Cause.(stackTracer); ok { + return tracer.StackTrace() + } + return nil +} + // ErrorHandler interface for handling an error for a // specific status code. type ErrorHandler func(int, error, Context) error @@ -165,8 +176,27 @@ type ErrorResponse struct { const defaultErrorCT = "text/html; charset=utf-8" -func defaultErrorHandler(status int, origErr error, c Context) error { +type stackTracer interface { + StackTrace() pkgError.StackTrace +} + +// Attempt to extract stacktrace error with stack if it implements stacktracer +func getTrace(err error) string { + + if errStack, ok := err.(stackTracer); ok { + return fmt.Sprintf("%+v", errStack.StackTrace()) + } else { + return err.Error() + } +} + +func isUnsafeEnvironment(c Context) bool { env := c.Value("env") + return env != nil && env.(string) != "development" && env.(string) != "test" +} + +func defaultErrorHandler(status int, origErr error, c Context) error { + requestCT := defaults.String(httpx.ContentType(c.Request()), defaultErrorCT) var defaultErrorResponse *ErrorResponse @@ -175,7 +205,7 @@ func defaultErrorHandler(status int, origErr error, c Context) error { c.Logger().Error(origErr) c.Response().WriteHeader(status) - if env != nil && env.(string) != "development" { + if isUnsafeEnvironment(c) { switch strings.ToLower(requestCT) { case "application/json", "text/json", "json", "application/xml", "text/xml", "xml": defaultErrorResponse = &ErrorResponse{ @@ -190,7 +220,7 @@ func defaultErrorHandler(status int, origErr error, c Context) error { } } - trace := origErr.Error() + trace := getTrace(origErr) if cause := errors.Unwrap(origErr); cause != nil { origErr = cause } diff --git a/errors_test.go b/errors_test.go index 10d359d03..fbabf3112 100644 --- a/errors_test.go +++ b/errors_test.go @@ -1,7 +1,9 @@ package buffalo import ( + errors "errors" "fmt" + "log" "net/http" "os" "testing" @@ -13,7 +15,7 @@ import ( "github.com/stretchr/testify/require" ) -//testLoggerHook is useful to test whats being logged. +// testLoggerHook is useful to test whats being logged. type testLoggerHook struct { errors []*logrus.Entry } @@ -65,6 +67,14 @@ func Test_defaultErrorHandler_Logger(t *testing.T) { r.Equal(http.StatusUnauthorized, testHook.errors[0].Data["status"]) } +func Test_defaultErrorHandler_JSON_test(t *testing.T) { + testDefaultErrorHandler(t, "application/json", "test") +} + +func Test_defaultErrorHandler_XML_test(t *testing.T) { + testDefaultErrorHandler(t, "text/xml", "test") +} + func Test_defaultErrorHandler_JSON_development(t *testing.T) { testDefaultErrorHandler(t, "application/json", "development") } @@ -94,7 +104,7 @@ func testDefaultErrorHandler(t *testing.T, contentType, env string) { app := New(Options{}) app.Env = env app.GET("/", func(c Context) error { - return c.Error(http.StatusUnauthorized, fmt.Errorf("boom")) + return c.Error(http.StatusUnauthorized, errors.New("boom")) }) w := httptest.New(app) @@ -108,18 +118,21 @@ func testDefaultErrorHandler(t *testing.T, contentType, env string) { ct := res.Header().Get("content-type") r.Equal(contentType, ct) b := res.Body.String() - - if env == "development" { + isDevOrTest := env == "development" || env == "test" + log.Printf(b) + if isDevOrTest { if contentType == "text/xml" { r.Contains(b, ``) r.Contains(b, `boom`) r.Contains(b, ``) r.Contains(b, ``) r.Contains(b, ``) + r.Contains(b, "github.com") // making sure trace is not empty } else { r.Contains(b, `"code":401`) r.Contains(b, `"error":"boom"`) r.Contains(b, `"trace":"`) + r.Contains(b, "github.com") // making sure trace is not empty } } else { if contentType == "text/xml" { diff --git a/go.mod b/go.mod index 93c6fe3d6..5eb33f3bb 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( github.com/gorilla/mux v1.8.0 github.com/gorilla/sessions v1.2.1 github.com/monoculum/formam v3.5.5+incompatible + github.com/pkg/errors v0.9.1 // indirect github.com/psanford/memfs v0.0.0-20210214183328-a001468d78ef github.com/sirupsen/logrus v1.9.0 github.com/spf13/cobra v1.6.1 diff --git a/go.sum b/go.sum index 5a7f7878a..09360c9f8 100644 --- a/go.sum +++ b/go.sum @@ -88,6 +88,8 @@ github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrk github.com/monoculum/formam v3.5.5+incompatible h1:iPl5csfEN96G2N2mGu8V/ZB62XLf9ySTpC8KRH6qXec= github.com/monoculum/formam v3.5.5+incompatible/go.mod h1:RKgILGEJq24YyJ2ban8EO0RUVSJlF1pGsEvoLEACr/Q= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= +github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= +github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/psanford/memfs v0.0.0-20210214183328-a001468d78ef h1:NKxTG6GVGbfMXc2mIk+KphcH6hagbVXhcFkbTgYleTI= From b126807f36546e9b9b70e65f21ebc12bf359f424 Mon Sep 17 00:00:00 2001 From: Piti Ongmongkolkul Date: Fri, 30 Dec 2022 17:25:04 +0700 Subject: [PATCH 2/3] fix: test with no form still show trace and name on development and test --- errors.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/errors.go b/errors.go index 89af5e638..c1ac4e4d0 100644 --- a/errors.go +++ b/errors.go @@ -247,7 +247,9 @@ func defaultErrorHandler(status int, origErr error, c Context) error { default: c.Response().Header().Set("content-type", defaultErrorCT) if err := c.Request().ParseForm(); err != nil { - trace = fmt.Sprintf("%s\n%s", err.Error(), trace) + trace = fmt.Sprintf("%s\n%s\n%s", err.Error(), origErr.Error(), trace) + } else { + trace = fmt.Sprintf("%s\n%s", origErr.Error(), trace) } routes := c.Value("routes") From 77c3a82922ee51d5ad1abb62c734684f50d1dd25 Mon Sep 17 00:00:00 2001 From: Piti Ongmongkolkul Date: Fri, 30 Dec 2022 17:37:54 +0700 Subject: [PATCH 3/3] remove extra print --- errors_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/errors_test.go b/errors_test.go index fbabf3112..38d55c27b 100644 --- a/errors_test.go +++ b/errors_test.go @@ -3,7 +3,6 @@ package buffalo import ( errors "errors" "fmt" - "log" "net/http" "os" "testing" @@ -119,7 +118,6 @@ func testDefaultErrorHandler(t *testing.T, contentType, env string) { r.Equal(contentType, ct) b := res.Body.String() isDevOrTest := env == "development" || env == "test" - log.Printf(b) if isDevOrTest { if contentType == "text/xml" { r.Contains(b, ``)