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

UrlDispatcher.add_routes to return list of resource objects #3866

Open
ntai opened this issue Jun 25, 2019 · 13 comments

Comments

@ntai
Copy link

commented Jun 25, 2019

Long story short

UrlDispatcher.add_routes returns nothing. I'd like to see a list of resources returned.

right now,

    def add_routes(self, routes):
        for route_obj in routes:
            route_obj.register(self)

What I want -

    def add_routes(self, routes):
        return [ route_obj.register(self) for route_obj in routes ]

Not sure route_obj.register returns a resource.

Expected behaviour

resources = router.add_routes(routes)

Actual behaviour

router.add_routes(routes) returns nothing

Steps to reproduce

This is an enhancement request.

Your environment

aiohttp 3.0.1
Python 3.6
Ubuntu 18.04 LTS

@asvetlov

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Why? What is your motivation?

@ntai

This comment has been minimized.

Copy link
Author

commented Jun 25, 2019

I can hand it over to cors to do post-add_route. Right now, I have to get to resource by app.router._resources.
if I'm adding multiple routes to top-level but with different cors, it's a chore to separate them out after add_routes called, but if add_routes returns a list of resources from the routes, it's a piece of cake.

    for resource in app.router.add_routes(routes):
      cors.add(resource, { '*': aiohttp_cors.ResourceOptions(allow_credentials=True, expose_headers="*", allow_headers="*") })
@asvetlov

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Not so easy.
First of all, add_routes() should return a list of routes if any, just by name.
Resource can have multiple routes, a route per supported HTTP method.
Secondly, now router.add_resoouce() can reuse the latest resource if possible. There is a case when add_routes() doesn't add a new resource at all.
Thirdly, there is app.router.resources() API, there is no need to access the private _resources method.

@ntai

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

Here is what I ended up doing to get around the aiohttp, aiohttp_cors and socket.io.

    app.router.add_routes(routes)
    app.router.add_static("/", rootdir)
    for resource in app.router._resources:
      if resource.raw_match("/socket.io/"):
        continue
      cors.add(resource, { '*': aiohttp_cors.ResourceOptions(allow_credentials=True, expose_headers="*", allow_headers="*") })
      pass

If I can get the resources back from add_routes, I don't have to exclude the "socket.io" to declare cors header.

if add_routes returns the newly added resources, that is perfectly fine. I didn't know it's not one-to-one match. The interaction between socket.io and aiohttp_cors is pretty cumbersome.

@asvetlov

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Please re-read my previous message:

  1. add_routes() adds routes and should return added routes, not resources. Otherwise, the behavior is confusing.
  2. add_routes() can be called multiple times. aiohttp doesn't forbid it.

Your code is perfectly fine if you manage access to the private variable:

    app.router.add_routes(routes)
    app.router.add_static("/", rootdir)
    for resource in app.router.resources():
      if resource.raw_match("/socket.io/"):
        continue
      cors.add(resource, { '*': aiohttp_cors.ResourceOptions(allow_credentials=True, expose_headers="*", allow_headers="*") })

aiohttp API should be self-consistent. Your proposal is not, sorry.

@ntai

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

class Resource(AbstractResource): add_route returns ResourceRoute object. I think what I'm asking is to return the list of ResourceRoute objects returned from add_routes.

@ntai

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

Let me ask this in different way - how can I turn the routes object to the list of resources? I need to hand the resources over to aiohttp_cors as cors.add() takes resource object.

My "workaround" is a result of digging into socketio to find out it is adding routes by itself to the aiohttp router. I shouldn't need to do this. I guess I don't care add_routes returns the list of resources or not. I need a way to convert the routes to the added resources without going into the router.resources(). How can I do this?

@ntai

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

aiohttp API should be self-consistent. Your proposal is not, sorry.

aiohttp_cors and aiohttp do not work well together. Even if aiohttp API may be consistent, if it's not working well with common pattern like implementing cors, the problem is in the API design of either aiohttp or aiohttp_cors, or both.

@asvetlov

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Sorry for the delay.
I'm ok with returning a list of AbstractRoute objects from add_routes().
Application.add_routes() should be updated as well.
The issue is waiting for a champion.

@ntai

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

Thanks for taking a look at this again. Here is a similar question asked by someone else. It is not obvious to figure this out.

Apache, lighttp, etc. can do this by a single line of config. In some sense, needing aiohttp_cors module is the problem itself. socketio.AsyncServer() has an option cors_allowed_origins='*'. I probably should rather ask to support router.add_route and/or router.add_routes takes an option cors_allowed_origins='*' so that I don't have to deal with aiohttp_cors to begin with.

@asvetlov

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

CORS is a complex protocol.
People use the very limited subset very often (Access-Control-Allow-Origin: * HTTP header).
But a good library should provide all features supported by the standard.
aiohttp-cors does it. The complex functionality prevents it from including into aiohttp core.

P.S.
socketio and aiohttp have different views on software design. I don't want to argue what way is better, people have different colors and habits you know. Personally I've found socketio approach simpler for newbies but much more limited.

@ZlatSic

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Hello!

I'd like to try to solve this issue, but I have some questions first. Since the register function doesn't return anything at the moment, should I modify it so it returns the created AbstractRoute? In that case, what about the StaticDef and the fact that both inherit AbstractRouteDef whose register function returns None at the moment? Should this None perhaps be modified to Any or Optional and then we could say that the register function returns the created resource?

@asvetlov

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Feel free to return routes (AbstractRoute perhaps) from methods that return nothing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.