-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
feat: [#521] User can custom recover when a request panic #103
Changes from all commits
e06c940
09245fe
5168d1e
32ee690
29d36ee
88daede
e82b340
f3efb50
61b20aa
a771bce
3b77bd0
cc7a261
21125ef
f3c666a
adf6a8c
0a3998c
d001e4d
28af043
b0a62cd
59ae234
5745c8f
fae6fbe
98f64a5
076581a
d5d8a93
998e3c4
9a3074c
a019972
3f2edef
f7a0ac6
36978b1
92c38d4
f66dadd
3915909
faf902e
cf0b756
0023e51
3c5f3bc
fb1faf6
8f27f62
49f6b21
0e34013
99e06c8
c6d79c8
e0727a7
8a0c44e
173e471
e4a1360
f30e2f4
6217114
35e0182
ace488a
f7f209e
440ba87
ca0c57f
4e8ddfb
dfa4bd0
44b41f6
4346508
1682fe7
60320c1
28dcae8
4aceba8
601dbd0
a79cae7
a581045
5a0101e
1c07943
f020930
a0b8834
47c53e7
343d493
41d57a1
057f865
4d82e79
e6fb55f
5df7701
aa56ca1
66e139e
46fc772
48144d5
72a5064
e7eef7b
ee526b3
4d7e660
5371bd7
cdf7d2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,11 +44,12 @@ func NewContextRequest(ctx *Context, log log.Log, validation contractsvalidate.V | |
request := contextRequestPool.Get().(*ContextRequest) | ||
httpBody, err := getHttpBody(ctx) | ||
if err != nil { | ||
log.Error(fmt.Sprintf("%+v", err)) | ||
LogFacade.Error(fmt.Sprintf("%+v", errors.Unwrap(err))) | ||
} | ||
request.ctx = ctx | ||
request.instance = ctx.instance | ||
request.httpBody = httpBody | ||
request.log = log | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch |
||
request.validation = validation | ||
return request | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,26 +20,15 @@ func Timeout(timeout time.Duration) contractshttp.Middleware { | |
done := make(chan struct{}) | ||
|
||
go func() { | ||
defer func() { | ||
if r := recover(); r != nil { | ||
if LogFacade != nil { | ||
LogFacade.Request(ctx.Request()).Error(r) | ||
} | ||
|
||
// TODO can be customized in https://github.com/goravel/goravel/issues/521 | ||
_ = ctx.Response().Status(http.StatusInternalServerError).String("Internal Server Error").Render() | ||
} | ||
|
||
close(done) | ||
}() | ||
|
||
defer HandleRecover(ctx, globalRecoverCallback) | ||
ctx.Request().Next() | ||
close(done) | ||
}() | ||
|
||
select { | ||
case <-done: | ||
case <-ctx.Request().Origin().Context().Done(): | ||
if errors.Is(ctx.Request().Origin().Context().Err(), context.DeadlineExceeded) { | ||
case <-timeoutCtx.Done(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch |
||
if errors.Is(timeoutCtx.Err(), context.DeadlineExceeded) { | ||
ctx.Request().AbortWithStatus(http.StatusGatewayTimeout) | ||
} | ||
} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test case to test the custom recover as well. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,13 +14,66 @@ import ( | |
"testing" | ||
"time" | ||
|
||
"github.com/gin-gonic/gin" | ||
"github.com/gin-gonic/gin/render" | ||
contractshttp "github.com/goravel/framework/contracts/http" | ||
"github.com/goravel/framework/contracts/validation" | ||
configmocks "github.com/goravel/framework/mocks/config" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestRecoverWithCustomCallback(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test case to test the default recover as well. |
||
mockConfig := configmocks.NewConfig(t) | ||
mockConfig.EXPECT().GetBool("app.debug").Return(true).Once() | ||
mockConfig.EXPECT().GetInt("http.drivers.gin.body_limit", 4096).Return(4096).Once() | ||
|
||
w := httptest.NewRecorder() | ||
req, _ := http.NewRequest("GET", "/recover", nil) | ||
|
||
route, err := NewRoute(mockConfig, nil) | ||
assert.Nil(t, err) | ||
|
||
globalRecover := func(ctx contractshttp.Context, err any) { | ||
ctx.Request().AbortWithStatusJson(http.StatusInternalServerError, gin.H{"error": "Internal Panic"}) | ||
} | ||
|
||
route.Recover(globalRecover) | ||
|
||
route.Get("/recover", func(ctx contractshttp.Context) contractshttp.Response { | ||
panic(1) | ||
}) | ||
|
||
route.ServeHTTP(w, req) | ||
|
||
assert.Equal(t, "{\"error\":\"Internal Panic\"}", w.Body.String()) | ||
assert.Equal(t, http.StatusInternalServerError, w.Code) | ||
|
||
mockConfig.AssertExpectations(t) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test case is pretty good. |
||
|
||
func TestRecoverWithDefaultCallback(t *testing.T) { | ||
mockConfig := configmocks.NewConfig(t) | ||
mockConfig.EXPECT().GetBool("app.debug").Return(true).Once() | ||
mockConfig.EXPECT().GetInt("http.drivers.gin.body_limit", 4096).Return(4096).Once() | ||
|
||
w := httptest.NewRecorder() | ||
req, _ := http.NewRequest("GET", "/recover", nil) | ||
|
||
route, err := NewRoute(mockConfig, nil) | ||
assert.Nil(t, err) | ||
|
||
route.Get("/recover", func(ctx contractshttp.Context) contractshttp.Response { | ||
panic(1) | ||
}) | ||
|
||
route.ServeHTTP(w, req) | ||
|
||
assert.Equal(t, "", w.Body.String()) | ||
assert.Equal(t, http.StatusInternalServerError, w.Code) | ||
|
||
mockConfig.AssertExpectations(t) | ||
} | ||
|
||
func TestFallback(t *testing.T) { | ||
mockConfig := &configmocks.Config{} | ||
mockConfig.EXPECT().GetBool("app.debug").Return(true).Once() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to modify this, I optimized the code in a previous PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please modify this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I need to return
log
?patch-3 of my fork currently uses
LogFacade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log
is fine, please update your branch.