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

Class based views are not supported #41

Closed
trivigy opened this issue Jun 21, 2016 · 13 comments
Closed

Class based views are not supported #41

trivigy opened this issue Jun 21, 2016 · 13 comments

Comments

@trivigy
Copy link

trivigy commented Jun 21, 2016

When using web.View as the handler if the route being added for '*' then cors doesn't work because the server seems not to be able to find the OPTIONS. Not sure why that is, I didn't take the time to investigate but my guess is that the * end up taking priority and the routing goes to the * handler instead of the CORS handler.

@rutsky
Copy link
Member

rutsky commented Aug 25, 2016

@trivigy thank you for posting this bug report and sorry for the long delay.

Yes, as I see * handler from class based view's (CBV) takes precedence over OPTIONS handler being added by aiohttp-cors, and I don't see easy way to handle CBV classes in aiohttp-cors so currently it will not work with them.

Perhaps best solution will be to introduce aiohttp-cors mixin for CBV which will handle OPTIONS and require users to inherit from it, like:

# Not implemented yet
from aiohttp_cors import ViewMixin

class MyView(ViewMixin, web.View):
    async def get(self):
        return await get_resp(self.request)

    async def post(self):
        return await post_resp(self.request)

@rutsky rutsky changed the title Weird issue with routing. Class based views are not supported Aug 25, 2016
@asvetlov
Copy link
Member

Makes sense

@balloob
Copy link

balloob commented Oct 18, 2016

I am using the following workaround to register my classes so that they work with aiohttp_cors:

def register_view(app, url, view):
    for method in ('get', 'post', 'delete', 'put'):
        handler = getattr(view, method, None)

        if not handler:
            continue

        app.router.add_route(method, url, handler)

@rutsky
Copy link
Member

rutsky commented Oct 18, 2016

@balloob will aiohttp_cors.ViewMixin mixin will help you if it will be implemented in aiohttp_cors?

@balloob
Copy link

balloob commented Oct 18, 2016

Probably - but I was already using views in a weird way: I wanted to instantiate the views before registering them so I could keep a reference to the state of the house (I'm working on Home Assistant). So I actually had a __call__ method on my instantiated view that would handle the routing to get/post etc. (I know that it's not the right way but we're transitioning from CherryPi WSGI server + Werkzeug and getting it to work is the first step.)

@asvetlov
Copy link
Member

@balloob if you want to instantiate your view classes before registering you don't need aiohttp.web.View at all -- custom classes may serve everything perfectly.

@trivigy
Copy link
Author

trivigy commented Mar 28, 2017

@rutsky Thanks for the suggestion. Took me a long time to notice that you responded. I actually use a very custom implementation of the View within our system. I personally wanted to add a whole lot more functionalities to the view and therefore re-implemented it. Had to override add_route() functions and all. Therefore this problem was never an issue for my systems but it was certainly a bug worth reporting.

@buhman
Copy link

buhman commented May 10, 2017

@balloob's hack works ok for me as well, but I think a ViewMixin would be better.

@asvetlov
Copy link
Member

asvetlov commented Aug 7, 2017

Guys, we need a champion for the issue.
Do anybody want work on PR?

@rutsky
Copy link
Member

rutsky commented Aug 8, 2017

I recently changed job, moved to another country and on paternity leave right now, so I don't have free time right now, sorry.

@pedrokiefer
Copy link
Contributor

pedrokiefer commented Dec 19, 2017

@asvetlov @rutsky would you please review my PR?

@asvetlov
Copy link
Member

Let me setup coverage tools first, I'll return to review after that.

@pedrokiefer
Copy link
Contributor

👍 💯

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

No branches or pull requests

6 participants