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

Allow route registration with decorator #1735

Closed
wants to merge 2 commits into from
Closed

Conversation

fafhrd91
Copy link
Member

I think it is useful, especially combined with sub-applications

@fafhrd91
Copy link
Member Author

@asvetlov @kxepal @popravich @socketpair thoughts?

router = mock.Mock(spec=MockAbstractRouter)
app = web.Application(router=router)

@app.route('/index.html')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my own impplementation of such decorator, I add reference to decorated method and all decorator's kwargs and args to some global array. And also I have a function that adds this saved information into application's routes.

In short (pseudocode):

global_array = []
def handler(*args, **kwargs):
    def decorator(function):
        global_array.append((function, args, kwargs))
        return function
    return decorator

@handler(....)
async def myhandler(request):
    ....

class MyApplication(web.Application):
    def add_saved_routes(self):
        for (method, args, kwargs):
            self.add_route(....)

def main():
    app = MyApplication()
    app.add_saved_routes()
    ...

This allows me to use decorators BEFORE creating application (during imports). This is especially important since I have many handlers which are placed in many files.

So, could you provide complete example showing usage of your decorators for the case when there are many files with handlers ?

@kxepal
Copy link
Member

kxepal commented Mar 21, 2017

I think this is a bad idea and goes against everything we have now. Routes as decorators forces you to use module-level scope to define application, loop and the rest of bits, returning you to the stone age flask-like design. While it's obliviously simple for starters, I don't think it's right and is good in mid/long term of application life.

Yesterday I meet one interesting issue with that kind of design using aiohttp: I investigated a project, where application and uvloop where defined at the module level. This module imported another one just before uvloop was set as the default, where Consul client was created as well at the module level. And since there was no loop argument passed for him, that client picked current default asyncio loop that was available - certainly not the uvloop one, since it was not yet even created. You may guess what happened next, when half of you application works with the one loop and some parts with the else one. And obliviously, author didn't even expected surprises of that kind.

I believe decorator routes will only make more situations like this.

For current design, I tend to think that Pyramid-style will fits more without much differences. For people who need flask-like style design, there is sanic already.

@jettify
Copy link
Member

jettify commented Mar 21, 2017

agree with @kxepal, there is even FAQ entry about such routing: http://aiohttp.readthedocs.io/en/stable/faq.html#are-there-any-plans-for-app-route-decorator-like-in-flask
Could this be implemented in separate package?

@popravich
Copy link
Member

Agree with @kxepal completely.
Such decorators should be used with extreme caution:
first you start with global app, loop, etc, etc; then your tests add more ifs to your app logic;
then import order begins to matter; next imports induce circular dependencies, and so on and so on.
This feature works until whole project fits single module.

@f0t0n
Copy link
Contributor

f0t0n commented Mar 21, 2017

Agreed with @kxepal.
I'd add that even in my Flask applications I never use such decorator-based routing but keep everything in the dict routing table such as the one below for the simple authentication service. Then just loading it with some simple functions to add routes to the application. The same I always did for the aiohttp-based projects.
So the routing table could be either nested to include sub-applications or separate tables for each sub-application, but anyway it's always explicit, declarative and configurable in one place. So it's easier to manage for any scale of projects.

from .public import session, signature, user

api_resources = {
    user: {
        'create': ('/users', ['POST'], {}),
        'verify': ('/users/verify/<uuid:user_id>/<string:verification_key>',
                   ['PATCH'],
                   {}),
    },
    session: {
        'create': ('/users/sessions', ['POST'], {}),
        'delete': ('/users/sessions', ['DELETE'], {}),
    },
    signature: {
        'verify': ('/signatures/verify', ['HEAD'], {}),
    },
}

@fafhrd91
Copy link
Member Author

sorry for delay.

I see two problems we discuss here.
@socketpair and @kxepal describe problem with configuration context. aiohttp does not have one and pyramid has it explicitly. and problem is worse with asyncio applications because of global event loop.
(that is the reason why I deprecated loop parameter in Application ctor). maybe we should introduce
configuration context, but that would be aiohttp 3.0 version.

I totally agree on configuration as a side effect of import problem, but I find Application.route() decorator super convenient. it allows to export module functionality in consolidated way (keeps context).
what if we limit route() decorator to one module, if you use it in one module you can not use it in another.
as @popravich mentioned router() decorator works when app fits into one module.

app configuration func could look like:

from . import users, content, ...

def build_app():
    app = web.Application()

    app.add_subapp('/users', users.USERS_APP)
    app.add_subapp('/content', content.CONTENT_APP)
    ...
    return app

@fafhrd91 fafhrd91 force-pushed the application-route branch from 820e413 to 34704ba Compare March 26, 2017 16:45
@fafhrd91
Copy link
Member Author

added one module usage limitation

@codecov-io
Copy link

codecov-io commented Mar 26, 2017

Codecov Report

Merging #1735 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
+ Coverage   97.41%   97.42%   +<.01%     
==========================================
  Files          37       37              
  Lines        7432     7445      +13     
  Branches     1288     1290       +2     
==========================================
+ Hits         7240     7253      +13     
  Misses         79       79              
  Partials      113      113
Impacted Files Coverage Δ
aiohttp/web.py 99.29% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c455db1...34704ba. Read the comment docs.

@kxepal
Copy link
Member

kxepal commented Mar 27, 2017

added one module usage limitation

I still think this is a bad move. Yes, this feature will allow us to have nice hello-world examples. Yes, this feature will make simple to make single modules web applications as like it was for single page web apps. But. If application will grow and user will be forced to split it into modules he/she will have to rewrite whole the routing system, application initialization and drop global context usage. And all this may not be quite oblivious. So there will be two chairs problem: or stay within a single module forever or cry for help.

Even for small applications, decorator-way may not be convenient. I'm working now on service which have only two routes. But, these routes are belongs to different API versions and I don't want to mix them or their logic in a single module because of coupling problem. So they are defined in two different modules, each of those defines how each API version should act.

There will be also a problem to use different routers with such kind of design. For instance, aiohttp_traversal. Decorator routes cannot be used with this one by design reasons.

@kxepal
Copy link
Member

kxepal commented Mar 27, 2017

I think we have here a lot of words against this feature. Is there any good reasons to have it?

@fafhrd91
Copy link
Member Author

Convenience, that is it, nothing else. I think we think too big, big applications, huge load, big teams. But I have a felling that in reality python get pushed back to area of glue language and data analytics. And most users just need to build something fast with minimal effort. Of course we can say f... them, learn how to write code, but again that is my feeling, those people are our primary audience. And It seems client part of aiohttp is more popular than server, which proves my point of view.

@fafhrd91
Copy link
Member Author

It is not about replacing normal registration with decorators, it is just adding ergonomics for small apps

@asvetlov
Copy link
Member

asvetlov commented Apr 9, 2017

Let's create a small third party library for route decorators, put it into aio-libs and reference from aiohttp documentation.

@fafhrd91
Copy link
Member Author

I don't like idea of separate repo. this feature is for new developers with prior flask and similar experience.
so it would be useless if you need to look for it. actually I think this feature could change how apps get build, one app per module, good isolation.

@fafhrd91 fafhrd91 closed this May 8, 2017
@asvetlov asvetlov deleted the application-route branch June 13, 2017 21:00
@lock
Copy link

lock bot commented Oct 28, 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 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants