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

router.add_resource(), regexps and curly braces #1778

Open
socketpair opened this Issue Apr 1, 2017 · 14 comments

Comments

Projects
None yet
5 participants
@socketpair
Contributor

socketpair commented Apr 1, 2017

Well. We stated in the docs:

image

What if regexp contains curly braces ?

Is it correct (now, and in future) to write regexps like:

resource = app.router.add_resource(r'/{name:\}+}')
resource = app.router.add_resource(r'/{name:[}]+}')

Technically speaking, they are correct regexps, but will aiohttp's resource format parser eat them?

In other words, it is not said which symbols (and how) must be escaped (from parser) in these regexps.

@justanr

This comment has been minimized.

justanr commented Apr 2, 2017

I've been able to use curly brackets in regexes, though this should be clarified in the documentation

@fafhrd91

This comment has been minimized.

Member

fafhrd91 commented Apr 6, 2017

could anyone create PR?

@socketpair

This comment has been minimized.

Contributor

socketpair commented Apr 6, 2017

Ans also, can anyone explain how route parsing is done ? regexps from sources are very complex. I can't prove that they don't have bugs.

@fafhrd91

This comment has been minimized.

Member

fafhrd91 commented Apr 8, 2017

I don't really know, @asvetlov wrote this part.

@fafhrd91

This comment has been minimized.

Member

fafhrd91 commented Apr 12, 2017

@asvetlov could you review this?

@justanr

This comment has been minimized.

justanr commented Apr 15, 2017

Doing some research, it looks like the two examples given in the original report produce invalid route errors -- e.g. the default aiohttp router refuses to build the route map. However, a route like {id:\d{4}} builds just fine.

Edit: Looks like the the ROUTE_RE pattern on the UrlDispatcher is what needs examining: r'(\{[_a-zA-Z][^{}]*(?:\{[^{}]*\}[^{}]*)*\})')

Any unmatched { or } in a routing regex will cause the build error -- the error message is a little cryptic though: ValueError: Invalid path '/'['+}']

@asvetlov

This comment has been minimized.

Member

asvetlov commented Apr 15, 2017

I'm inclining to forbidding curved brackets inside embedded regexps.

@justanr

This comment has been minimized.

justanr commented Apr 15, 2017

I think that's a move too far, though I could be bias since I have date based routes that use the {m,n} matching. I could move the actual matching groups inside the handler, but if the router can ward off unnecessary matches for me I'd rather use that.

@socketpair

This comment has been minimized.

Contributor

socketpair commented Apr 16, 2017

Maybe, we should invent some escaping? if so, any regexp can be used.

@socketpair

This comment has been minimized.

Contributor

socketpair commented Apr 16, 2017

For example, in sed, regexp borders may be specified by ANY symbol. for example, /regexp/ or |regexp| or even @regexp@. Additionally, if same (as border one) symbol should be used in regexp, it should be escaped using backslash, i.e. @some\@regexp@ is exactly the same as |some@regexp|

@fafhrd91

This comment has been minimized.

Member

fafhrd91 commented Apr 16, 2017

Is there any good routing open source libraries? I would replace our own implementation with some good library

@kxepal

This comment has been minimized.

Member

kxepal commented Apr 16, 2017

I don't think there is a reason to do that for current router. It has own flaws, but afaik it was never pretended to be very fast, optimal and featured. Just the one that works in most of cases and easy to be implemented.

Instead of making it more complicated and slower, may be take a look on possible alternatives?

P.S. As for this issue, it seems to be possible solved by working with path segments instead of whole path and few more stringy work to decouple identifier and regexp matching. Curved brackets there are just a marker to strip and go.

@kxepal

This comment has been minimized.

Member

kxepal commented Apr 16, 2017

@fafhrd91
Few from my bookmarks:
https://github.com/c9s/r3
https://github.com/nitely/kua

Though, not sure about good.

@fafhrd91

This comment has been minimized.

Member

fafhrd91 commented Apr 16, 2017

I prefer c-library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment