DRAFT: contrib/dimfeld/httptreemux.v5: fix resource name for 301 redirects #14
Conversation
| return "", false | ||
| // TODO: what about the other two router.RedirectBehavior cases? | ||
| if lr.StatusCode == http.StatusMovedPermanently && router.RedirectBehavior == httptreemux.Redirect301 { | ||
| if strings.HasSuffix(route, "/") { |
There was a problem hiding this comment.
This is for the original case where:
- request URL:
GET /api/example/(with trailing slash) - matched handler:
GET /api/example(without trailing slash)
| if !found { | ||
| return "", false | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This is for the other possible case where:
- request URL:
GET /api/example(without trailing slash) - matched handler:
GET /api/example/(with trailing slash)
| routeLen := len(route) | ||
| trailingSlash := route[routeLen-1] == '/' && routeLen > 1 | ||
| if trailingSlash { | ||
| route = strings.TrimSuffix(route, "/") |
There was a problem hiding this comment.
This is to standardize the resource name.
For handler GET /api/example both of these cases will now have the same resource name:
GET /api/example(without trailing slash)GET /api/example/(with trailing slash)
Without this change you can end up with a scenario where you have two different resources in Datadog:
GET /api/example(without trailing slash)GET /api/example/(with trailing slash)
I believe this is a safe change to make because the router panics if you try and register two handlers with the same path with only the trailing slash being the difference, i.e.:
router := New()
router.GET("/api/example", handler200)
// this panics
router.GET("/api/example/", handler200)
func handler200(w http.ResponseWriter, _ *http.Request, _ map[string]string) {
w.Write([]byte("OK\n"))
}
| // if the route does not have a trailing slash, retry lookup without trailing slash | ||
| rReq := req.Clone(req.Context()) | ||
| rReq.RequestURI = rReq.RequestURI + "/" | ||
| rReq.URL.Path = rReq.URL.Path + "/" | ||
|
|
||
| lr, found = router.Lookup(w, rReq) | ||
| if !found { | ||
| return "", false | ||
| } |
There was a problem hiding this comment.
I'm a little confused by this. The comment says if the route does not have a trailing slash, to retry the lookup without the trailing slash, but this code looks like it's adding the slash
There was a problem hiding this comment.
Eep! Yep. That's definitely a typo. Thanks for pointing it out!
devillecodes
force-pushed
the
dv/remove-trailing-slash-in-resource-name
branch
4 times, most recently
from
c339239
to
83c7e7c
Compare
The httptreemux router has a set of redirect behaviours that is invoked when a request URL and matched route only differs in a trailing slash. The default behaviour is to respond with a 301 (moved permanently) to redirect the client to the exact path of the matched handler. We previously patched one of the two scenarios where this occurs in DataDog#2332. The changes in this commit addresses the other scenario. We also standardize the resource name in the event that there is a trailing slash missmatch between request URL and matched handler. We always truncate the trailing slash in the resource name. Fixes DataDog#2663
devillecodes
force-pushed
the
dv/remove-trailing-slash-in-resource-name
branch
from
83c7e7c
to
d5ec356
Compare
|
Closing this draft in favour of the upstream PR: DataDog#2664. |
DO NOT MERGE.