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.Application and web_urldispatcher.UrlDispatcher can't be used separately #1373

Closed
ikalnytskyi opened this issue Nov 5, 2016 · 14 comments
Labels
Milestone

Comments

@ikalnytskyi
Copy link

Long story short

Since aiohttp 1.1.0 there's no correct way to create web.Application and web_urldispatcher.UrlDispatcher separately, since the latest requires the former as argument and both saves each other as protected properties.

app = web.Application(
    router=web_urldispatcher.UrlDispatcher(app),  # app doesn't exist yet!
)

possible solutions is to patch protected property:

# either on router
app = web.Application(router=web_urldispatcher.UrlDispatcher(None))
app.router._app = app

# or on application
app = web.Application()
app._router = web_urldispatcher.UrlDispatcher(app)

aiohttp users can't rely on protected attributes, since they may changed from version to version.

Expected behaviour

There should be a proper way to resolve this solution.

Actual behaviour

Workaround is needed.

Steps to reproduce

  • Create web_urldispatcher.UrlDispatcher independently and pass it to web.Application.

Your environment

  • aiohttp 1.1.0
  • macOS sierra, Arch Linux
@asvetlov
Copy link
Member

asvetlov commented Nov 6, 2016

What's your use case for deriving from UrlDispatcher?
From my perspective you should either use it as is or reimplement all functionality by supporting AbstractRouter ABC.

@ikalnytskyi
Copy link
Author

@asvetlov I don't derive from UrlDispatcher, I just create it explicitly with some factory function. I mean in my case I construct router instance and pass it to application instance, and do not follow pattern used in the docs:

app = web.Application()
app.router.add_route(...)

Since there's a problem with that pattern either - add_route is not in AbstractRouter interface, thus I can't use it if I may have non-standard router one day (and I actually do).

@asvetlov
Copy link
Member

asvetlov commented Nov 8, 2016

Hmm, but why do you need to create UrlDispatcher manually?
Please don't get me wrong: adding Router.setup_app(app) is easy and could solve your issue.
But maybe you just misuse aiohttp Public API?
For example I pretty sure that @toumorokoshi shouldn't derive from UrlDispatcher as I've answered.
At list my suggestion is right from my point of view :)
Honestly I prefer aggregation instead of inheritance and aiohttp is built in this matter: end users shouldn't derive from aiohttp classes unless these ones are not Abstract Base Classes.
It helps to keep API clean and allows to perform very deep aiohttp refactoring without raising unexpected backward compatibility problems.

Thus what is your use case? Would you provide a simple code snippet with problem demonstration?
The only reason that I can imagine is making declarative style for route registering.
If it's true we might work on adding it into aiohttp itself.
At early aiohttp stages I was reluctant for adding declarative style -- I had striven to keep routing API as minimal as possible.
But now this API is stabilized more or less, we could make next step.

@ikalnytskyi
Copy link
Author

@asvetlov Who's talking about derivation? :) I'm talking about factory function that somehow gather and return a router that I need to use with my application. Let's say, map endpoints to classes from YAML or create various router classes based on some parameter in the config. To me it sounds like a valid case especially taking into account that web.Application receives router as its parameter. As for router class, I thought UrlDispatcher is a part of public aiohttp API and I can use this class to create UrlDispatcher instance (again, not inherit), can't I? If it's internal API, well, ok, I can close this issue as it's my issue then.

Regarding my particular case. I've decided to use router as a way to do API versioning. In my case I have VersionRouter that inherits from AbstractRouter and implements resolve method. resolve method, on the other side, delegates its work to a concrete UrlDispatcher (i.e. I have multiple UrlDispatcher instances each tracks resources of a certain API version). I've played around with aiohttp in my project and that way to solve versioning problem seemed reasonable to me. Here's an example: https://github.com/xsnippet/xsnippet-api/blob/master/xsnippet_api/router.py#L55-L82

@toumorokoshi
Copy link
Contributor

I closed my ticket #1365, but I think I'll join in on this.

It would be nice to be able to instantiate UrlDispatchers separately, or rely on that behavior. For my project (aiohttp-transmute), my job becomes a lot easier if I can rely on a dispatcher that handles basic mounting of routes resolving to the appropriate path.

I guess if UrlDispatcher isn't supposed to be extended (or public), I would just continue to build my tooling around it's methods anyway, since that's the default router and I'm going to guess that a lot of people will have a painful upgrade if the default router behavior changed.

Just some thoughts. Maybe you can say that you're not supposed to extend from it, but it's pretty "public" already.

@fafhrd91
Copy link
Member

fafhrd91 commented Nov 8, 2016

actually UrlDispatcher doesnt use _app except in add_subapp and then it uses application's protected method. this method has code smell feeling.
should add_subapp be part of Application object?

@fafhrd91
Copy link
Member

fafhrd91 commented Nov 8, 2016

@asvetlov ^^^

@ikalnytskyi
Copy link
Author

Just some thoughts. Maybe you can say that you're not supposed to extend from it, but it's pretty "public" already.

+1 on this. The thing is, UrlDispatcher is doing a great job resolving routes to views, and implementing a brand new resolver each time I need a slightly different router doesn't seem right. So I bet everyone will try to reuse UrlDispatcher either via inheriting or aggregating.

@asvetlov
Copy link
Member

asvetlov commented Nov 9, 2016

@fafhrd91 add_subapp() cannot be moved to Application -- it creates UrlDispatcher routes.
What I want to do now is collecting all use broken cases.
https://github.com/xsnippet/xsnippet-api/blob/master/xsnippet_api/router.py#L55-L82 is good example, let me sleep on it.

From my perspective UrlDispatcher should be sealed class.

@roadsideseb
Copy link

I'm having the same issue that I am overwriting UrlDispatcher and would like to pass it into the Application as router but can't because of the required app argument in UrlDispatcher.

My use case for this is adding route-based validation of incoming request using JSON schema. Being able to subclass UrlDispatcher makes it possible to store route-schema mapping on the dispatcher and validate them in a middleware.

If subclassing UrlDispatcher and passing it into the app at initialization via the router argument, what is the use case for having that argument? Is it to support the sub-app functionality?

ikalnytskyi pushed a commit to xsnippet/xsnippet-api that referenced this issue Dec 7, 2016
Since aiohttp 1.1 some backward incompatible changes have been
implemented. Despite each of them has its own issue on GitHub:

  - aio-libs/aiohttp#1373
  - aio-libs/aiohttp#1416

it's not clear when they become resolved. In order to be able to
fly on latest aiohttp version, this commit resolves those issues
by implementing workarounds on our side.
@asvetlov asvetlov added this to the 1.2 milestone Dec 7, 2016
@asvetlov
Copy link
Member

asvetlov commented Dec 7, 2016

Fixed by #1458

@asvetlov asvetlov closed this as completed Dec 7, 2016
@roadsideseb
Copy link

That's awesome! Thanks a lot!

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 4, 2017

UrlDispatcher does not depend on Application anymore

@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

5 participants