Route matching problems if %+ already populated (issue 1187) #1188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is for Issue #1187. It's not safe to look at `%+` without first having checked whether the regex matched successfully or not. Otherwise, we might see keys from a previous regex match, because `%+` is only cleared after a successful match. (perldoc perlvar confirms, "Note: "%-" and "%+" are tied views into a common internal hash associated with the last successful regular expression.") Dancer2 doesn't have problems here, as it already returns immediately if the regex didn't match, the same way this change does. Note: I think this would only happen via certain deployment methods, and if there was an earlier route which used named captures. In particular, we never had any problems in our large API app's comprehensive test suite when we used Dancer::Test. We've re-worked our testing framework to use Plack::Test, partially in readyness to switch to Dancer2, and started getting weird failures which were tracked down to incorrect route matching, and this was the cause.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is for Issue #1187.
It's not safe to look at
%+without first having checked whether the regexmatched successfully or not. Otherwise, we might see keys from a previous regex
match, because
%+is only cleared after a successful match.(perldoc perlvar confirms, "Note: "%-" and "%+" are tied views into a common
internal hash associated with the last successful regular expression.")
Dancer2 doesn't have problems here, as it already returns immediately if the
regex didn't match, the same way this change does.
Note: I think this would only happen via certain deployment methods, and if
there was an earlier route which used named captures.
In particular, we never had any problems in our large API app's comprehensive
test suite when we used Dancer::Test. We've re-worked our testing framework to
use Plack::Test, partially in readyness to switch to Dancer2, and started
getting weird failures which were tracked down to incorrect route matching, and
this was the cause.