fix Weighting calculation for path converters and add tests #3018
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
Fix issue pallets#2924 where Rules with dynamic elements would take priority over rules with static elements of the same length. The underlying issue was that the ordering for rules was backwards with regard to number of argument weights. In order to ensure that static elements match first the shortest rule must be tested first. To ensure that a RulePart that contains a path converter does not incorrectly take priority over other RuleParts that are part of other Rules that should have priority, RuleParts containing a path converter are assigned an infinite number of argument weights because they can consume an arbitrary number of url path elements when matching. With this change, two consecutive path converters give priority to the first converter. In general two consecutive path converters with different names cannot have consistent or predicatable behavior (where would you cut?). Tests are updated accordingly. Might consider making back to back path converters an error.
|
Closing following discussion in #2924. |
|
I don't think this should be closed because this fixes issues beyond those described in #2924. Without the fix that I implement here it is impossible to get correct behavior and I and anyone else affected would have to carry a patch. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fix issue #2924 where Rules with dynamic elements would take priority over rules with static elements of the same length.
The underlying issue was that the ordering for rules was backwards with regard to number of argument weights. In order to ensure that static elements match first the shortest rule must be tested first.
To ensure that a RulePart that contains a path converter does not incorrectly take priority over other RuleParts that are part of other Rules that should have priority, RuleParts containing a path converter are assigned an infinite number of argument weights because they can consume an arbitrary number of url path elements when matching.
With this change, two consecutive path converters give priority to the first converter. In general two consecutive path converters with different names cannot have consistent or predicatable behavior (where would you cut?). Tests are updated accordingly. Might consider making back to back path converters an error.
fixes #2924