Allow SkipClean to be evaluated on each route independently. #463
Conversation
|
Thank you. I’m really behind in review backlog, but a quick ask: also
document the “existing children” behavior in the godoc for SkipClean?
Bonus points for adding an example to the README too.
…On Thu, Mar 21, 2019 at 7:40 AM George Vilches ***@***.***> wrote:
This provides a solution to #460
<#460> . Cleaning is now moved into
the route matcher, and is only activated if SkipClean has been set for that
specific router (whether explicitly, or by inheritance from a parent route
upon creation).
Note that due to the lack of parent knowledge in a Route/Router (which was
removed a few months ago), SkipClean() changes only apply to routes that
were created after the SkipClean change. Since we have no handle or
reference to a Subrouter() once it's been created from the parent router
(it's only stored as a matcher interface), we can't make SkipClean()
trickle down to the already existing children. New ones will pick it up
though. Since this is how other current properties behave, this seemed to
be expected behavior.
------------------------------
You can view, comment on, or merge this pull request online at:
#463
Commit Summary
- Allow SkipClean to be evaluated on each route independently.
File Changes
- *M* mux.go <https://github.com/gorilla/mux/pull/463/files#diff-0>
(40)
- *M* mux_test.go
<https://github.com/gorilla/mux/pull/463/files#diff-1> (34)
- *M* regexp.go <https://github.com/gorilla/mux/pull/463/files#diff-2>
(17)
- *M* route.go <https://github.com/gorilla/mux/pull/463/files#diff-3>
(1)
Patch Links:
- https://github.com/gorilla/mux/pull/463.patch
- https://github.com/gorilla/mux/pull/463.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#463>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABIcHrNMoxNmfrtroTy9JR-Er5jUm8nks5vY5nVgaJpZM4cBlwI>
.
|
| @@ -413,6 +406,13 @@ type RouteMatch struct { | |||
| Handler http.Handler | |||
| Vars map[string]string | |||
|
|
|||
| // Cleaned version of the path being matched. | |||
There was a problem hiding this comment.
Since these are exported, can you provide clearer guidance for a user trying to understand either:
- What their value is?
- Should I be setting this?
- What the default is?
| @@ -170,7 +171,23 @@ func (r *routeRegexp) Match(req *http.Request, match *RouteMatch) bool { | |||
| if r.options.useEncodedPath { | |||
| path = req.URL.EscapedPath() | |||
| } | |||
| return r.regexp.MatchString(path) | |||
| if !r.options.skipClean { | |||
There was a problem hiding this comment.
Please document this code-path more clearly - e.g. "why".
|
Hey @gavbaa - are you still interested in finishing this one off? |
|
Yes, just have to set some time aside to go through and address the comments. |
|
Is there any way that I can help to get this merged? I need this!! |
|
@theverything you could create your own branch and complete the remainder of the work if you'd like. |
mux.go
Outdated
| w.Header().Set("Location", url.String()) | ||
| w.WriteHeader(http.StatusMovedPermanently) |
There was a problem hiding this comment.
Have you considered using http.Redirect here instead? Would be more consistent with what other projects are doing.
Basically the difference is that it would give a response body with the location in too and a moved permanently message.
|
Hey @gavbaa, @theverything - are you guys working on this PR? |
Co-authored-by: Jarri Abidi <jarri1337@gmail.com> Signed-off-by: Corey Daley <cdaley@redhat.com>
|
@gavbaa If you would like to resolve the conflict with this pull request we can work on getting it reviewed and merged. Thanks |
|
Just noting no response as yet - will plan on coming back around to this if there's further updates. |
This provides a solution to #460 . Cleaning is now moved into the route matcher, and is only activated if SkipClean has been set for that specific router (whether explicitly, or by inheritance from a parent route upon creation).
Note that due to the lack of parent knowledge in a Route/Router (which was removed a few months ago), SkipClean() changes only apply to routes that were created after the SkipClean change. Since we have no handle or reference to a Subrouter() once it's been created from the parent router (it's only stored as a
matcherinterface), we can't make SkipClean() trickle down to the already existing children. New ones will pick it up though. Since this is how other current properties behave, this seemed to be expected behavior.