Logger skip paths allow params #3144
Conversation
hanyucui
force-pushed
the
skips-use-fullpath
branch
from
259a946
to
0407401
Compare
hanyucui
changed the title
hanyucui
force-pushed
the
skips-use-fullpath
branch
from
ff9cc35
to
e08f574
Compare
hanyucui
changed the title
This is more flexible and is breaking only in very rare cases. Restore original behavior Update logger path skipping tests
hanyucui
force-pushed
the
skips-use-fullpath
branch
from
e08f574
to
40991b4
Compare
|
@appleboy @thinkerou Please take a look. Thanks in advance! |
1 similar comment
|
@appleboy @thinkerou Please take a look. Thanks in advance! |
|
|
||
| PerformRequest(router, "GET", "/logged") | ||
| assert.Contains(t, buffer.String(), "200") | ||
|
|
||
| buffer.Reset() | ||
| PerformRequest(router, "GET", "/loggedUsers/2") | ||
| assert.Contains(t, buffer.String(), "200") |
There was a problem hiding this comment.
assert.Contains(t, "200", buffer.String())
There was a problem hiding this comment.
Thanks for leaving your feedback @appleboy. I just want to confirm this is the change your want, since the current code base adopts the convention of assert.Contains(t, buffer.String(), "200"). I will change them all once confirmed. Let me know.
There was a problem hiding this comment.
@hanyucui sorry for my mistake. you are right.
func Contains(t TestingT, s, contains interface{}, msgAndArgs ...interface{}) boolThere was a problem hiding this comment.
Thanks for double-checking, @appleboy. Let me know if other changes are needed.
| router.GET("/skipped", func(c *Context) {}) | ||
| router.GET("/skippedUsers/:id", func(c *Context) {}) | ||
|
|
||
| PerformRequest(router, "GET", "/logged") | ||
| assert.Contains(t, buffer.String(), "200") |
There was a problem hiding this comment.
assert.Contains(t, "200", buffer.String())
| router.GET("/skipped", func(c *Context) {}) | ||
| router.GET("/skippedUsers/:id", func(c *Context) {}) | ||
|
|
||
| PerformRequest(router, "GET", "/logged") | ||
| assert.Contains(t, buffer.String(), "200") |
There was a problem hiding this comment.
assert.Contains(t, "200", buffer.String())
|
|
||
| PerformRequest(router, "GET", "/logged") | ||
| assert.Contains(t, buffer.String(), "200") | ||
|
|
||
| buffer.Reset() | ||
| PerformRequest(router, "GET", "/loggedUsers/2") | ||
| assert.Contains(t, buffer.String(), "200") |
There was a problem hiding this comment.
assert.Contains(t, "200", buffer.String())
|
Change milestone to v1.10 |
This change adds support for skip paths with params, e.g.,
"/user/:id". This is more flexible and the implementation is simply
comparing
Context.FullPath()rather thanContext.Request.URL.Pathagainst skip paths.
The only case where it would lead to regression is where legacy code
uses hardcoded paths for parameterized paths. E.g., when hardcoded
paths like "/user/1" and "/user/2" are added to skipped paths for
"/user/:id". Although legal, I cannot imagine when such a case
would be useful. Hence I think regression would be extremely rare in
practice.
This change also updates relevant tests.