Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(DefaultHTTPErrorHandler): return error message when message is an error #2456

Merged
merged 4 commits into from
May 29, 2023
Merged

fix(DefaultHTTPErrorHandler): return error message when message is an error #2456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94f9c21
fix(DefaultHTTPErrorHandler): return error message when message is an…
gnuletik May 26, 2023
321c93e
fix(echo): marshal as JSON for types that implements json.Marshaler
gnuletik May 28, 2023
f133ab6
test(DefaultHTTPErroHandler): refactor to table based test
gnuletik May 28, 2023
e7e0db9
fix(DefaultHTTPErrorHandler): remove useless break statement in switc…
gnuletik May 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion echo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ package echo
import (
stdContext "context"
"crypto/tls"
"encoding/json"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -438,12 +439,18 @@ func (e *Echo) DefaultHTTPErrorHandler(err error, c Context) {
// Issue #1426
code := he.Code
message := he.Message
if m, ok := he.Message.(string); ok {

switch m := he.Message.(type) {
case string:
if e.Debug {
message = Map{"message": m, "error": err.Error()}
} else {
message = Map{"message": m}
}
case json.Marshaler:
// do nothing - this type knows how to format itself to JSON
case error:
aldas marked this conversation as resolved.
Show resolved Hide resolved
message = Map{"message": m.Error()}
}

// Send response
Expand Down
192 changes: 133 additions & 59 deletions echo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1286,67 +1286,141 @@ func TestHTTPError_Unwrap(t *testing.T) {
})
}

type customError struct {
s string
}

func (ce *customError) MarshalJSON() ([]byte, error) {
return []byte(fmt.Sprintf(`{"x":"%v"}`, ce.s)), nil
}

func (ce *customError) Error() string {
return ce.s
}

func TestDefaultHTTPErrorHandler(t *testing.T) {
e := New()
e.Debug = true
e.Any("/plain", func(c Context) error {
return errors.New("an error occurred")
})
e.Any("/badrequest", func(c Context) error {
return NewHTTPError(http.StatusBadRequest, "Invalid request")
})
e.Any("/servererror", func(c Context) error {
return NewHTTPError(http.StatusInternalServerError, map[string]interface{}{
"code": 33,
"message": "Something bad happened",
"error": "stackinfo",
})
})
e.Any("/early-return", func(c Context) error {
err := c.String(http.StatusOK, "OK")
if err != nil {
assert.Fail(t, err.Error())
}
return errors.New("ERROR")
})
e.GET("/internal-error", func(c Context) error {
err := errors.New("internal error message body")
return NewHTTPError(http.StatusBadRequest).SetInternal(err)
})
var testCases = []struct {
name string
givenDebug bool
whenPath string
expectCode int
expectBody string
}{
{
name: "with Debug=true plain response contains error message",
givenDebug: true,
whenPath: "/plain",
expectCode: http.StatusInternalServerError,
expectBody: "{\n \"error\": \"an error occurred\",\n \"message\": \"Internal Server Error\"\n}\n",
},
{
name: "with Debug=true special handling for HTTPError",
givenDebug: true,
whenPath: "/badrequest",
expectCode: http.StatusBadRequest,
expectBody: "{\n \"error\": \"code=400, message=Invalid request\",\n \"message\": \"Invalid request\"\n}\n",
},
{
name: "with Debug=true complex errors are serialized to pretty JSON",
givenDebug: true,
whenPath: "/servererror",
expectCode: http.StatusInternalServerError,
expectBody: "{\n \"code\": 33,\n \"error\": \"stackinfo\",\n \"message\": \"Something bad happened\"\n}\n",
},
{
name: "with Debug=true if the body is already set HTTPErrorHandler should not add anything to response body",
givenDebug: true,
whenPath: "/early-return",
expectCode: http.StatusOK,
expectBody: "OK",
},
{
name: "with Debug=true internal error should be reflected in the message",
givenDebug: true,
whenPath: "/internal-error",
expectCode: http.StatusBadRequest,
expectBody: "{\n \"error\": \"code=400, message=Bad Request, internal=internal error message body\",\n \"message\": \"Bad Request\"\n}\n",
},
{
name: "with Debug=false the error response is shortened",
whenPath: "/plain",
expectCode: http.StatusInternalServerError,
expectBody: "{\"message\":\"Internal Server Error\"}\n",
},
{
name: "with Debug=false the error response is shortened",
whenPath: "/badrequest",
expectCode: http.StatusBadRequest,
expectBody: "{\"message\":\"Invalid request\"}\n",
},
{
name: "with Debug=false No difference for error response with non plain string errors",
whenPath: "/servererror",
expectCode: http.StatusInternalServerError,
expectBody: "{\"code\":33,\"error\":\"stackinfo\",\"message\":\"Something bad happened\"}\n",
},
{
name: "with Debug=false when httpError contains an error",
whenPath: "/error-in-httperror",
expectCode: http.StatusBadRequest,
expectBody: "{\"message\":\"error in httperror\"}\n",
},
{
name: "with Debug=false when httpError contains an error",
whenPath: "/customerror-in-httperror",
expectCode: http.StatusBadRequest,
expectBody: "{\"x\":\"custom error msg\"}\n",
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
e := New()
e.Debug = tc.givenDebug // With Debug=true plain response contains error message

// With Debug=true plain response contains error message
c, b := request(http.MethodGet, "/plain", e)
assert.Equal(t, http.StatusInternalServerError, c)
assert.Equal(t, "{\n \"error\": \"an error occurred\",\n \"message\": \"Internal Server Error\"\n}\n", b)
// and special handling for HTTPError
c, b = request(http.MethodGet, "/badrequest", e)
assert.Equal(t, http.StatusBadRequest, c)
assert.Equal(t, "{\n \"error\": \"code=400, message=Invalid request\",\n \"message\": \"Invalid request\"\n}\n", b)
// complex errors are serialized to pretty JSON
c, b = request(http.MethodGet, "/servererror", e)
assert.Equal(t, http.StatusInternalServerError, c)
assert.Equal(t, "{\n \"code\": 33,\n \"error\": \"stackinfo\",\n \"message\": \"Something bad happened\"\n}\n", b)
// if the body is already set HTTPErrorHandler should not add anything to response body
c, b = request(http.MethodGet, "/early-return", e)
assert.Equal(t, http.StatusOK, c)
assert.Equal(t, "OK", b)
// internal error should be reflected in the message
c, b = request(http.MethodGet, "/internal-error", e)
assert.Equal(t, http.StatusBadRequest, c)
assert.Equal(t, "{\n \"error\": \"code=400, message=Bad Request, internal=internal error message body\",\n \"message\": \"Bad Request\"\n}\n", b)

e.Debug = false
// With Debug=false the error response is shortened
c, b = request(http.MethodGet, "/plain", e)
assert.Equal(t, http.StatusInternalServerError, c)
assert.Equal(t, "{\"message\":\"Internal Server Error\"}\n", b)
c, b = request(http.MethodGet, "/badrequest", e)
assert.Equal(t, http.StatusBadRequest, c)
assert.Equal(t, "{\"message\":\"Invalid request\"}\n", b)
// No difference for error response with non plain string errors
c, b = request(http.MethodGet, "/servererror", e)
assert.Equal(t, http.StatusInternalServerError, c)
assert.Equal(t, "{\"code\":33,\"error\":\"stackinfo\",\"message\":\"Something bad happened\"}\n", b)
e.Any("/plain", func(c Context) error {
return errors.New("an error occurred")
})

e.Any("/badrequest", func(c Context) error { // and special handling for HTTPError
return NewHTTPError(http.StatusBadRequest, "Invalid request")
})

e.Any("/servererror", func(c Context) error { // complex errors are serialized to pretty JSON
return NewHTTPError(http.StatusInternalServerError, map[string]interface{}{
"code": 33,
"message": "Something bad happened",
"error": "stackinfo",
})
})

// if the body is already set HTTPErrorHandler should not add anything to response body
e.Any("/early-return", func(c Context) error {
err := c.String(http.StatusOK, "OK")
if err != nil {
assert.Fail(t, err.Error())
}
return errors.New("ERROR")
})

// internal error should be reflected in the message
e.GET("/internal-error", func(c Context) error {
err := errors.New("internal error message body")
return NewHTTPError(http.StatusBadRequest).SetInternal(err)
})

e.GET("/error-in-httperror", func(c Context) error {
return NewHTTPError(http.StatusBadRequest, errors.New("error in httperror"))
})

e.GET("/customerror-in-httperror", func(c Context) error {
return NewHTTPError(http.StatusBadRequest, &customError{s: "custom error msg"})
})

c, b := request(http.MethodGet, tc.whenPath, e)
assert.Equal(t, tc.expectCode, c)
assert.Equal(t, tc.expectBody, b)
})
}
}

func TestEchoClose(t *testing.T) {
Expand Down