Skip to content

Commit a260cf5

Browse files
aldasaldas
authored
Merge pull request #2900 from aldas/v5_resolveresponsestatus
Add ResolveResponseStatus function to help middleware/handlers determine HTTP status code and echo.Response
2 parents 9183f1e + 19364e2 commit a260cf5

File tree

4 files changed

+148
-46
lines changed

4 files changed

+148
-46
lines changed

‎httperror.go‎

Lines changed: 60 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,21 +9,6 @@ import (
99
"net/http"
1010
)
1111

12-
// HTTPStatusCoder is interface that errors can implement to produce status code for HTTP response
13-
type HTTPStatusCoder interface {
14-
StatusCode() int
15-
}
16-
17-
// StatusCode returns status code from error if it implements HTTPStatusCoder interface.
18-
// If error does not implement the interface it returns 0.
19-
func StatusCode(err error) int {
20-
var sc HTTPStatusCoder
21-
if errors.As(err, &sc) {
22-
return sc.StatusCode()
23-
}
24-
return 0
25-
}
26-
2712
// Following errors can produce HTTP status code by implementing HTTPStatusCoder interface
2813
var (
2914
ErrBadRequest = &httpError{http.StatusBadRequest} // 400
@@ -50,6 +35,66 @@ var (
5035
ErrInvalidListenerNetwork = errors.New("invalid listener network")
5136
)
5237

38+
// HTTPStatusCoder is interface that errors can implement to produce status code for HTTP response
39+
type HTTPStatusCoder interface {
40+
StatusCode() int
41+
}
42+
43+
// StatusCode returns status code from error if it implements HTTPStatusCoder interface.
44+
// If error does not implement the interface it returns 0.
45+
func StatusCode(err error) int {
46+
var sc HTTPStatusCoder
47+
if errors.As(err, &sc) {
48+
return sc.StatusCode()
49+
}
50+
return 0
51+
}
52+
53+
// ResolveResponseStatus returns the Response and HTTP status code that should be (or has been) sent for rw,
54+
// given an optional error.
55+
//
56+
// This function is useful for middleware and handlers that need to figure out the HTTP status
57+
// code to return based on the error that occurred or what was set in the response.
58+
//
59+
// Precedence rules:
60+
// 1. If the response has already been committed, the committed status wins (err is ignored).
61+
// 2. Otherwise, start with 200 OK (net/http default if WriteHeader is never called).
62+
// 3. If the response has a non-zero suggested status, use it.
63+
// 4. If err != nil, it overrides the suggested status:
64+
// - StatusCode(err) if non-zero
65+
// - otherwise 500 Internal Server Error.
66+
func ResolveResponseStatus(rw http.ResponseWriter, err error) (resp *Response, status int) {
67+
resp, _ = UnwrapResponse(rw)
68+
69+
// once committed (sent to the client), the wire status is fixed; err cannot change it.
70+
if resp != nil && resp.Committed {
71+
if resp.Status == 0 {
72+
// unlikely path, but fall back to net/http implicit default if handler never calls WriteHeader
73+
return resp, http.StatusOK
74+
}
75+
return resp, resp.Status
76+
}
77+
78+
// net/http implicit default if handler never calls WriteHeader.
79+
status = http.StatusOK
80+
81+
// suggested status written from middleware/handlers, if present.
82+
if resp != nil && resp.Status != 0 {
83+
status = resp.Status
84+
}
85+
86+
// error overrides suggested status (matches typical Echo error-handler semantics).
87+
if err != nil {
88+
if s := StatusCode(err); s != 0 {
89+
status = s
90+
} else {
91+
status = http.StatusInternalServerError
92+
}
93+
}
94+
95+
return resp, status
96+
}
97+
5398
// NewHTTPError creates new instance of HTTPError
5499
func NewHTTPError(code int, message string) *HTTPError {
55100
return &HTTPError{

‎httperror_test.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,3 +107,80 @@ func TestStatusCode(t *testing.T) {
107107
})
108108
}
109109
}
110+
111+
func TestResolveResponseStatus(t *testing.T) {
112+
someErr := errors.New("some error")
113+
114+
var testCases = []struct {
115+
name string
116+
whenResp http.ResponseWriter
117+
whenErr error
118+
expectStatus int
119+
expectResp bool
120+
}{
121+
{
122+
name: "nil resp, nil err -> 200",
123+
whenResp: nil,
124+
whenErr: nil,
125+
expectStatus: http.StatusOK,
126+
expectResp: false,
127+
},
128+
{
129+
name: "resp suggested status used when no error",
130+
whenResp: &Response{Status: http.StatusCreated},
131+
whenErr: nil,
132+
expectStatus: http.StatusCreated,
133+
expectResp: true,
134+
},
135+
{
136+
name: "error overrides suggested status with StatusCode(err)",
137+
whenResp: &Response{Status: http.StatusAccepted},
138+
whenErr: ErrBadRequest,
139+
expectStatus: http.StatusBadRequest,
140+
expectResp: true,
141+
},
142+
{
143+
name: "error overrides suggested status with 500 when StatusCode(err)==0",
144+
whenResp: &Response{Status: http.StatusAccepted},
145+
whenErr: ErrInternalServerError,
146+
expectStatus: http.StatusInternalServerError,
147+
expectResp: true,
148+
},
149+
{
150+
name: "nil resp, error -> 500 when StatusCode(err)==0",
151+
whenResp: nil,
152+
whenErr: someErr,
153+
expectStatus: http.StatusInternalServerError,
154+
expectResp: false,
155+
},
156+
{
157+
name: "committed response wins over error",
158+
whenResp: &Response{Committed: true, Status: http.StatusNoContent},
159+
whenErr: someErr,
160+
expectStatus: http.StatusNoContent,
161+
expectResp: true,
162+
},
163+
{
164+
name: "committed response with status 0 falls back to 200 (defensive)",
165+
whenResp: &Response{Committed: true, Status: 0},
166+
whenErr: someErr,
167+
expectStatus: http.StatusOK,
168+
expectResp: true,
169+
},
170+
{
171+
name: "resp with status 0 and no error -> 200",
172+
whenResp: &Response{Status: 0},
173+
whenErr: nil,
174+
expectStatus: http.StatusOK,
175+
expectResp: true,
176+
},
177+
}
178+
for _, tc := range testCases {
179+
t.Run(tc.name, func(t *testing.T) {
180+
resp, status := ResolveResponseStatus(tc.whenResp, tc.whenErr)
181+
182+
assert.Equal(t, tc.expectResp, resp != nil)
183+
assert.Equal(t, tc.expectStatus, status)
184+
})
185+
}
186+
}

‎middleware/request_logger.go‎

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,8 @@ type RequestLoggerConfig struct {
160160
LogReferer bool
161161
// LogUserAgent instructs logger to extract request user agent values.
162162
LogUserAgent bool
163-
// LogStatus instructs logger to extract response status code. If handler chain returns an echo.HTTPError,
164-
// the status code is extracted from the echo.HTTPError returned
163+
// LogStatus instructs logger to extract response status code. If handler chain returns an error,
164+
// the status code is extracted from the error satisfying echo.StatusCoder interface.
165165
LogStatus bool
166166
// LogContentLength instructs logger to extract content length header value. Note: this value could be different from
167167
// actual request body size as it could be spoofed etc.
@@ -211,7 +211,7 @@ type RequestLoggerValues struct {
211211
Referer string
212212
// UserAgent is request user agent values.
213213
UserAgent string
214-
// Status is response status code. Then handler returns an echo.HTTPError then code from there.
214+
// Status is a response status code. When the handler returns an error satisfying echo.StatusCoder interface, then code from it.
215215
Status int
216216
// Error is error returned from executed handler chain.
217217
Error error
@@ -272,7 +272,6 @@ func (config RequestLoggerConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
272272
}
273273

274274
req := c.Request()
275-
res := c.Response()
276275
start := now()
277276

278277
if config.BeforeNextFunc != nil {
@@ -284,6 +283,7 @@ func (config RequestLoggerConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
284283
// checked with `c.Response().Committed` field.
285284
c.Echo().HTTPErrorHandler(c, err)
286285
}
286+
res := c.Response()
287287

288288
v := RequestLoggerValues{
289289
StartTime: start,
@@ -330,26 +330,16 @@ func (config RequestLoggerConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
330330
v.UserAgent = req.UserAgent()
331331
}
332332

333-
var resp *echo.Response
334333
if config.LogStatus || config.LogResponseSize {
335-
if r, err := echo.UnwrapResponse(res); err != nil {
336-
c.Logger().Error("can not determine response status and/or size. ResponseWriter in context does not implement unwrapper interface")
337-
} else {
338-
resp = r
339-
}
340-
}
334+
resp, status := echo.ResolveResponseStatus(res, err)
341335

342-
if config.LogStatus {
343-
v.Status = -1
344-
if resp != nil {
345-
v.Status = resp.Status
336+
if config.LogStatus {
337+
v.Status = status
346338
}
347-
if err != nil && !config.HandleError {
348-
// this block should not be executed in case of HandleError=true as the global error handler will decide
349-
// the status code. In that case status code could be different from what err contains.
350-
var hsc echo.HTTPStatusCoder
351-
if errors.As(err, &hsc) {
352-
v.Status = hsc.StatusCode()
339+
if config.LogResponseSize {
340+
v.ResponseSize = -1
341+
if resp != nil {
342+
v.ResponseSize = resp.Size
353343
}
354344
}
355345
}
@@ -359,12 +349,6 @@ func (config RequestLoggerConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
359349
if config.LogContentLength {
360350
v.ContentLength = req.Header.Get(echo.HeaderContentLength)
361351
}
362-
if config.LogResponseSize {
363-
v.ResponseSize = -1
364-
if resp != nil {
365-
v.ResponseSize = resp.Size
366-
}
367-
}
368352
if logHeaders {
369353
v.Headers = map[string][]string{}
370354
for _, header := range headers {

‎router_test.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3079,10 +3079,6 @@ func TestDefaultRouter_AddDuplicateRouteNotAllowed(t *testing.T) {
30793079
assert.Equal(t, "OLD", body)
30803080
}
30813081

3082-
func TestName(t *testing.T) {
3083-
3084-
}
3085-
30863082
// See issue #1531, #1258 - there are cases when path parameter need to be unescaped
30873083
func TestDefaultRouter_UnescapePathParamValues(t *testing.T) {
30883084
var testCases = []struct {

0 commit comments

Comments
 (0)