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

web.View and multiple route resolution issue #931

Closed
trivigy opened this issue Jun 18, 2016 · 6 comments
Closed

web.View and multiple route resolution issue #931

trivigy opened this issue Jun 18, 2016 · 6 comments

Comments

@trivigy
Copy link

trivigy commented Jun 18, 2016

I have been messing around with the web.View and trying to get similar super convenient functionality as the one that exists in flask. I am talking about dynamic routes variables resolving to handler arguments in the view. Basically the idea is somewhat like this. I would declare a route as such:

app.router.add_route(
    path='/users/{user_id}',
    handler=view.User,
    name='users',
    methods=['GET', 'PATCH'],
    defaults={'user_id': 1}
)
app.router.add_route(
    path='/users',
    handler=view.User,
    name='users',
    methods=['POST']
)

Once that works I would like the handler to be able to access those via the argument signature as follows (module view.py):

class Users(web.View):
    async def get(user_id):
        pass

    async def post(user_id):
        pass

    async def patch():
        pass

Few points to mention about the current version not supporting it and then will follow up with a few questions and proposal. Foremost since this logic will try to declare two resources with the same name this will fail. Assuming I fit fix this by overriding UrlDispatcher and implement my own add_route I could potentially attach a single resource with multiple route however there is another issue. It seems that when the resolver is trying to resolve the routes (web_urldispatcher:Resource.resolve) it only acutally checks for the last acceptable method to return. Otherwise it returns no UrlMappingMatchInfo with the allowed_methods. Here is the code:

    @asyncio.coroutine
    def resolve(self, method, path):
        allowed_methods = set()

        match_dict = self._match(path)
        if match_dict is None:
            return None, allowed_methods

        for route in self._routes:
            route_method = route.method
            allowed_methods.add(route_method)

            # This is where the issue is <<<<<<----------------------------------------
            if route_method == method or route_method == hdrs.METH_ANY:
                return UrlMappingMatchInfo(match_dict, route), allowed_methods
        else:
            return None, allowed_methods

Since I really would like this kind of functionality I am happy to implement it but if this is not something in line with where the project is moving I would rather implement a wrapping solution personally since I want to continue being up to date with the version releases. Would love some feedback on this, thanks.

@trivigy
Copy link
Author

trivigy commented Jun 18, 2016

Okay so I misunderstood the logic on the resolution part It actually works the way it should. My bad. But the functionality question still stands.

@asvetlov
Copy link
Member

Honestly I don't follow what functionality are you requesting

@trivigy
Copy link
Author

trivigy commented Jun 19, 2016

I am really requesting a functionality. Already implemented it on my end by inheriting bunch of the classes and changing some stuff. Was asking to figure out whether you guys would be interested in the functionality in which case I would spend the extra time writing it into the application instead of overloading stuff.

Functionality from flask. When you add dynamic route the groups that get selected in the path of that route during resolution become the function arguments of the handler that is attached.

Example:

app.router.add_route(
    path='/users/{user_id}',
    handler=users,
    name='users'

async def users(request, user_id):   <<<<------------ argument
        pass
)

In the current aiohttp it only funnels request object through and does not support "default" values for those arguments. Personally I like that functionality and have implemented it for myself. Was asking if I should spend the time porting it into the application for pull request.

@asvetlov
Copy link
Member

asvetlov commented Aug 14, 2016

@trivigy very sorry for late answer.
At very early times, event before aiohttp.web appearing we have experimented with passing route matches as web-handler parameters.
But we've found the way is just wrong.
It's impossible to mix at least three different namespace in the same web-handler signature.
What's if user want to name his path template as /users/{request}?
The name conflicts with reguar request parameter.
Moreover, the pages also accept query params (/path/to?a=1&b=2).
Why not parse query part of URL and pass it as function arguments?
The query part may have multiple values for the same key, you know. How to handle it?
So far so bad.

We decided to have the only request param for web-handler.
All additional information can be retrieved from request object by request.match_info and request.GET.

If you are able to make your preferable way on top of aiohttp.web -- that's perfectly fine.
It is an evidence of aiohttp flexibility.

But aiohttp as the library will never have this functionality, sorry.
Please just live with it.

@trivigy
Copy link
Author

trivigy commented Aug 15, 2016

No worries at all. I tinker and change for my own personal preference. I'll keep asking and proposing as I go. I will definitely just live with it.

@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.
Projects
None yet
Development

No branches or pull requests

2 participants