Skip to content

Commit

Permalink
fix Weighting calculation for path converters and add tests
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tgbugs committed Jan 16, 2025
1 parent 7868bef commit 2193620
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 3 deletions.
6 changes: 4 additions & 2 deletions src/werkzeug/routing/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,7 @@ def _parse_rule(self, rule: str) -> t.Iterable[RulePart]:
self._trace.append((False, data["static"]))
content += data["static"] if static else re.escape(data["static"])

haspath = False
if data["variable"] is not None:
if static:
# Switching content to represent regex, hence the need to escape
Expand All @@ -640,6 +641,7 @@ def _parse_rule(self, rule: str) -> t.Iterable[RulePart]:
convertor_number += 1
argument_weights.append(convobj.weight)
self._trace.append((True, data["variable"]))
haspath = data["converter"] == "path"

if data["slash"] is not None:
self._trace.append((False, "/"))
Expand All @@ -651,7 +653,7 @@ def _parse_rule(self, rule: str) -> t.Iterable[RulePart]:
weight = Weighting(
-len(static_weights),
static_weights,
-len(argument_weights),
float("+inf") if haspath else len(argument_weights),
argument_weights,
)
yield RulePart(
Expand Down Expand Up @@ -681,7 +683,7 @@ def _parse_rule(self, rule: str) -> t.Iterable[RulePart]:
weight = Weighting(
-len(static_weights),
static_weights,
-len(argument_weights),
float("+inf") if haspath else len(argument_weights),
argument_weights,
)
yield RulePart(
Expand Down
45 changes: 44 additions & 1 deletion tests/test_routing.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,20 +381,63 @@ def test_greedy():
[
r.Rule("/foo", endpoint="foo"),
r.Rule("/<path:bar>", endpoint="bar"),
r.Rule("/<path:bar>/<path:blub>", endpoint="bar"),
r.Rule("/<path:bar>/<blub>", endpoint="bar"),
r.Rule("/<baz>/static", endpoint="oops"),
]
)
adapter = map.bind("example.org", "/")

assert adapter.match("/foo") == ("foo", {})
assert adapter.match("/blub") == ("bar", {"bar": "blub"})
assert adapter.match("/he/he") == ("bar", {"bar": "he", "blub": "he"})
assert adapter.match("/he/static") == ("oops", {"baz": "he"})

assert adapter.build("foo", {}) == "/foo"
assert adapter.build("bar", {"bar": "blub"}) == "/blub"
assert adapter.build("bar", {"bar": "blub", "blub": "bar"}) == "/blub/bar"


def test_greedy_double_paths():
# two back to back paths do not have any meaning and should
# probably cause an error in _parse_rule
map = r.Map(
[
r.Rule("/foo", endpoint="foo"),
r.Rule("/<path:bar>", endpoint="bar"),
r.Rule("/<path:bar>/<path:blub>", endpoint="bar"),
]
)
adapter = map.bind("example.org", "/")

assert adapter.match("/foo") == ("foo", {})
assert adapter.match("/blub") == ("bar", {"bar": "blub"})
assert adapter.match("/he/he") == ("bar", {"bar": "he/he"})
# can't match ("bar", {"bar": "he", "blub": "he"}) without breaking static matching
# use a rule like "/<path:bar/<blub>" instead

assert adapter.build("foo", {}) == "/foo"
assert adapter.build("bar", {"bar": "blub"}) == "/blub"
assert adapter.build("bar", {"bar": "blub", "blub": "bar"}) == "/blub/bar"


def test_static_priority():
# see https://github.com/pallets/werkzeug/issues/2924
map = r.Map(
[
r.Rule("/<path:dyn2>/<dyn1>", endpoint="file"),
r.Rule("/<dyn1>/statn", endpoint="stat"),
],
)

adapter = map.bind("example.org", "/")

assert adapter.match("/d2/d1", method="GET") == (
"file",
{"dyn2": "d2", "dyn1": "d1"},
)
assert adapter.match("/d1/statn", method="GET") == ("stat", {"dyn1": "d1"})


def test_path():
map = r.Map(
[
Expand Down

0 comments on commit 2193620

Please sign in to comment.