Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paths are not matched in order #3

Closed
plowman opened this issue Apr 23, 2013 · 2 comments
Closed

Paths are not matched in order #3

plowman opened this issue Apr 23, 2013 · 2 comments
Assignees

Comments

@plowman
Copy link
Contributor

plowman commented Apr 23, 2013

from clastic import Application, default_response 

def api(api_path):
    return 'api: %s' % api_path 

def two_segments(one, two):
    return 'two_segments: %s, %s' % (one, two) 

def three_segments(one, two, three):
    return 'three_segments: %s, %s, %s' % (one, two, three) 

routes = [
    ('/api/<path:api_path>', api, default_response),
    ('/<one>/<two>', two_segments, default_response),
    ('/<one>/<two>/<three>', three_segments, default_response),
]
app = Application(routes)
app.serve()

Visiting /api/ results in a 404, which is arguably wrong depending on we think a "" should work.
Visiting /api/a results in "api: a", which is right.
Visiting /api/a/b results in "three_segments: api, a, b", which is batshit crazy.
Visiting /api/a/b/c results in "api: a/b/c", which is right.

Mahmoud said:

Before matching, it calls update: https://github.com/mitsuhiko/werkzeug/blob/master/werkzeug/routing.py#L1198

which sorts by: https://github.com/mitsuhiko/werkzeug/blob/master/werkzeug/routing.py#L770

which yields:
app._rules
[<Route '/_meta/json/' - <function get_routes_info at 0x7f657e674848,
<Route '/_meta/' - <function get_routes_info at 0x7f657e674848,
<Route '/<one/<two/<three' <function three_segments at 0x7f657e681848,
<Route '/api/<api_path' - <function api at 0x7f657e681758,
<Route '/<one/<two' <function two_segments at 0x7f657e6817d0]

@mahmoud
Copy link
Owner

mahmoud commented Apr 23, 2013

Yes, this was definitely a good catch and, yes, sadly it is an issue in Werkzeug. In researching the problem, I was further disappointed to see it's been a known issue for over a year: pallets/werkzeug#185

I'm not surprised they haven't fixed it, though; the sorting seems to have been part of routing system's design. A misguided one, I believe, but someone, somewhere is relying on this behavior, bafflingly enough.

For those too lazy to click the links in my explanation above, TL;DR:

Regardless of insertion order, Werkzeug sorts routes by the number of arguments and "weight". Weight is the heuristically computed complexity of checking if a route matches. This includes going as deep as a path segment being more expensive to check than an integer segment.

As to why sorting is misguided:

  • At the API level, Werkzeug's Map of URL Rules accepts a list, which I perceive as conveying "order matters". Every other developer I've polled agrees.
  • At the behavioral level, sorting clearly breaks expectations. More importantly, it's almost completely out of developer control, as the Map flags it for resorting every time the list of rules/routes changes.
  • At the performance level, I've never seen an application affected by routing time, especially when the route semantics don't allow for straight up regexes, which is something I appreciate about Werkzeug.

I mean, how many routes did the Pocoo dudes have? 200+? The sorting functionality should be a util function at best. Let application developers take responsibility for performance concerns.

Anyhoo, I'm working around this right now. Seemingly every web framework complicates routing, but not Clastic, anymore: Clastic routes will stay in insertion order.

@mahmoud
Copy link
Owner

mahmoud commented Apr 23, 2013

Alright, fix is pushed, I'll update PyPI shortly.

Also, re: the 404, I know it's a little bit confusing, but it's consistent with all the other segment converters. Also, it usually works out such that root urls like example.com/api/ host some sort of landing page or docs, so there aren't many lines saved.

You can, of course, reuse the same endpoint function by setting a default on that argument. The original endpoint function:

def api(api_path):
    return 'api: %s' % api_path 

Reusable endpoint function, via default value for the api_path argument:

def api(api_path=None):
    if api_path is None:
        return 'api: api_path is None'
    return 'api: %s' % api_path 

Thanks again for the great catch/report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants