Fix issue 200, fail fast if regex is incorrectly specified using capturing groups #218
Conversation
|
|
||
| // Check for capturing groups which used to work in older versions | ||
| if reg.NumSubexp() != len(idxs)/2 { | ||
| panic("Regexp appears to be using capturing groups. To fix, see: https://github.com/gorilla/mux/issues/200") |
There was a problem hiding this comment.
We shouldn't link to the issue. We should instead show the failing route - "route %q contains capture groups in its regexp. Only non-capturing groups are accepted: e.g. (?:pattern) instead of (pattern)"
(or an approximation of that)
| @@ -35,8 +35,10 @@ type routeTest struct { | |||
| path string // the expected path of the match | |||
| pathTemplate string // the expected path template to match | |||
| hostTemplate string // the expected host template to match | |||
| additionalPath string // additional path to add to route (and parse) during test | |||
There was a problem hiding this comment.
Can this not be rolled into an existing field for testing? (just wary of adding once-off test fields)
There was a problem hiding this comment.
Agreed, I don't think it's necessary at all. Since calling Path panics right away, that can just be done as its own test. Not all the tests need to follow the exact same structure and go through testRoute.
This way shouldPanic can also be removed.
There was a problem hiding this comment.
I just added an alternative test - and I didn't see this last comment before I pushed it.
Let me know if you're happy with the alternative which I think is probably more reusable than it previously was, or if you'd like me to just test the parsing separately and remove the additional fields.
| @@ -1389,6 +1391,30 @@ func TestSubrouterErrorHandling(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // See: https://github.com/gorilla/mux/issues/200 | |||
| func TestPanicOnCapturingGroups(t *testing.T) { | |||
There was a problem hiding this comment.
Just rewrite this function:
defer func() {
if recover() == nil {
t.Errorf("(%v) Expected panic, however test completed sucessfully.\n", test.title)
}
}()
r.NewRoute().Path("/{type:(promo|special)}/{promoId}.json")
The rest of the test changes can then be removed. There are already tests for capturing groups.
|
Simplified test as suggested, and rebased to squash commits. |
|
LGTM. @elithrar merge when ready :) |
|
Thanks @aeijdenberg - appreciate it! |
|
You're welcome @elithrar - I think I spent most of a morning debugging an issue that ended up being due to this (after I upgraded a heap of vendored dependencies), so hopefully this saves someone else that time. :) |
…pturing groups. (gorilla#218)" This reverts commit 392c28f.
…pturing groups. (gorilla#218)" This reverts commit 392c28f.
No description provided.