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

Rooted UrlDispatcher / Mini Applications #780

Closed
frederikaalund opened this issue Feb 15, 2016 · 16 comments
Closed

Rooted UrlDispatcher / Mini Applications #780

frederikaalund opened this issue Feb 15, 2016 · 16 comments
Labels

Comments

@frederikaalund
Copy link
Contributor

Currently, each Application has a single router instance (UrlDispatcher by default). This works fine for small applications.

Express (of node.js) has the convenient router class which expands on this functionality. Specifically, Express allows the user to create "mini applications" which can be assigned (re-rooted) to a given path. I think that aiohttp could benefit from similar functionality.

That is, instead of having the following calls:

    app.router.add_route('GET', '/blog/posts', posts_handler)
    app.router.add_route('POST', '/blog/authors', authors_handler)
    app.router.add_route('*', '/blog/about', about_handler)
    ...

One could have something like a rooted UrlDispatcher / mini application:

    Class Blog(RootedUrlDispatcher):
        def __init__(self):
            super().add_route('GET', '/posts', self.posts_handler)
            super().add_route('POST', '/authors', self.authors_handler)
            super().add_route('*', '/about', self.about_handler)
            ...

and then later simply:

    app.router.add_router('/blog', Blog())

Note that all the calls to /blog/* will be forwarded to the Blog class. Thus one can keep all the resources (e.g., database, template files) belonging to the blog inside the Blog class. Also, one can easily re-root the entire Blog "mini application" by changing only a single line of code.

The RootedUrlDispatcher is just a suggestion. Any way of implementing mini applications would be a nice addition to aiohttp.

@asvetlov
Copy link
Member

I thought about problem like this for a long time.
Latest aiohttp 0.21 is one step closer to solution.
Eventually I'll implement nested applications.
Stay tune.

@frederikaalund
Copy link
Contributor Author

@asvetlov Thanks for the quick reply! I'm glad to hear that you are already working on a solution. Looking forward to the next couple of releases.

I'd like to add that if the user could modify the Request object (#682) then the user could implement this feature himself.

@asvetlov
Copy link
Member

Do you mean request.path replacing?

I doubt if it good idea, better to support nested routes with unmodified paths given from request.

Otherwise printing debug info with unprefixed paths makes a mess.

@frederikaalund
Copy link
Contributor Author

Do you mean request.path replacing?

Yes, that is what I had in mind.

I doubt if it good idea, better to support nested routes with unmodified paths given from request.

I completely agree. I'm just impatient and want a workaround. It's a much better idea to support nested routes directly (and leave the request object immutable).

@Gr1N
Copy link

Gr1N commented Apr 9, 2016

Hi, it would be great to have ability to include many routes in one (like in Django :).
I tried to write my own realization of this feature (https://github.com/Gr1N/wuffi/blob/master/wuffi/core/urls.py#L14):

class UrlDispatcher(web_urldispatcher.UrlDispatcher):
    def include_routes(self, path, router):
        if not path.startswith('/'):
            raise ValueError('path should be started with /')
        elif path.endswith('/'):
            raise ValueError('path should not be ended with /')

        router = import_string('{}.router'.format(router))

        # include `resources`
        self._resources.extend(self._to_include(path, *router._resources))

        # include `named_resources`
        for name, resource in router._named_resources.items():
            if name in self._named_resources:
                raise ValueError('Duplicate {!r}, '
                                 'already handled by {!r}'
                                 .format(name, self._named_resources[name]))

            self._named_resources[name] = resource

    def _to_include(self, path, *resources):
        for resource in resources:
            if isinstance(resource, web_urldispatcher.PlainResource):
                resource._path = '{}{}'.format(path, resource._path)
            elif isinstance(resource, web_urldispatcher.DynamicResource):
                resource._pattern, resource._formatter = self._compile_path(
                    '{}{}'.format(path, resource._formatter))

        return resources

    def _compile_path(self, path):
        pattern = ''
        formatter = ''
        for part in self.ROUTE_RE.split(path):
            match = self.DYN.match(part)
            if match:
                pattern += '(?P<{}>{})'.format(match.group('var'), self.GOOD)
                formatter += '{' + match.group('var') + '}'
                continue

            match = self.DYN_WITH_RE.match(part)
            if match:
                pattern += '(?P<{var}>{re})'.format(**match.groupdict())
                formatter += '{' + match.group('var') + '}'
                continue

            if '{' in part or '}' in part:
                raise ValueError("Invalid path '{}'['{}']".format(path, part))

            formatter += part
            pattern += re.escape(part)

        try:
            compiled = re.compile('^' + pattern + '$')
        except re.error as exc:
            raise ValueError(
                "Bad pattern '{}': {}".format(pattern, exc)) from None

        return compiled, formatter

@asvetlov what do you think about this solution? To my mind it is very ugly and buggy, but it works for my case. If you have idea how to write it in right way, but have no time for realization, so tell us, maybe we can try to write smth and send PR.

@cynecx
Copy link

cynecx commented Oct 8, 2016

May I ask what the status on this "nested"-router feature is?

@asvetlov
Copy link
Member

asvetlov commented Oct 8, 2016

I'm working on it.
The nested route is a big challenge but please keep fingers crossed.
I really want to have the feature in aiohttp 1.1

@asvetlov
Copy link
Member

Fixed by #1301

@Gr1N
Copy link

Gr1N commented Oct 27, 2016

@asvetlov thanks for realization, as I can see from documentation we need to create additional application and add those application to existing route, to my mind it's a complex solution, what about just creating a new UrlDispatcher and then adding to existing route? Something like this:

router = UrlDispatcher()
router.add_route('*', '/view1', View1)

sub_router = UrlDispatcher()
sub_router.add_route('*', '/view2', View2)

router.add_subrouter('/sub', sub_router)

You solution very useful in cases when we need something big with middlewares, signals, etc. But in many cases we need something simple like in my example.

I can try to create solution like I see it and send you PR, what do you think?

@asvetlov
Copy link
Member

No. Subapplications are required exactly because app is an container for its global state, signals and middlewares.
If you need just resource grouping you always could write a helper like

def fill_routes(app, prefix):
     app.router.add_get(prefix+'/getter', handler1)
     app.router.add_post(prefix+'/setter', handler2)

fill_routes(app, '/prefix')

Sub-routers make overcomplication.

@Gr1N
Copy link

Gr1N commented Oct 27, 2016

Looks like solution, but not very useful to my mind. Let's imaging big routing configuration, this is example using django realization, but it pretty clear show what I'm talking about:

url(r'^top/', include([
    url(r'^$', view1, name='view1'),

    url(r'^sub1/', include([
        url(r'^$', view2, name='view2'),
        url(r'^view3/$', view3, name='view3'),
    ], namespace='sub1')),

    url(r'^sub2/(?P<sub2_id>[0-9a-z-]{36})/', include([
        url(r'^$', view4, name='view4'),
        url(r'^view5/$', view5, name='view5'),
        url(r'^sub3/(?P<sub3_id>[0-9a-z-]{36})/', include([
            url(r'^$', view6, name='view6'),
        ], namespace='sub3')),
    ], namespace='sub2')),
], namespace='top'))

It's easy to read and I can reverse urls using: top:sub1:view3 and top:sub2:sub3:view6.

Using your way I need to write additional helpers (sometimes very complex) in each project.

I know aiohttp not a django, but for now routing in aiohttp very simple (and not useful for big projects) and requires improvements to my mind, but maybe I'm wrong.

@asvetlov
Copy link
Member

Are you really asking for nested routes or namespaces for url reversing?
What the principal difference between router.add_subapp() and proposed router.add_subrouter()?

@Gr1N
Copy link

Gr1N commented Oct 27, 2016

Why I want router.add_subrouter(), for example I have some project:

/users
   views.py
   router.py
/news
   views.py
   routes.py
main.py
routes.py

In top level router I just want something like this:

# routes.py
from users.routes import router as users_router
from news.routes import router as news_router

router = UrlDispatcher()
router.add_subrouter('/users', users_router, name_or_namespace='users')
router.add_subrouter('/news', news_router, name_or_namespace='news')

# main.py
from routes import router
app = Application(router=router)
...

In this case I don't want to create additional apps for news and users modules, because I don't need additional middlewares or signals, I just want to include routes. And yes if we can include routes I want to use namespaces for url reversing.

My proposal is more about project organization and how aiohttp router can help me with this.

@asvetlov
Copy link
Member

Well, I need to sleep on it.
Please create a new issue with your proposal.

@Gr1N
Copy link

Gr1N commented Oct 28, 2016

Done #1343

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants