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

Unused response_factory argument of web.UrlDispatcher.add_static/web.StaticResource #2290

Closed
zemlanin opened this issue Sep 25, 2017 · 7 comments

Comments

@zemlanin
Copy link

Long story short

Argument response_factory doesn't affect static responses

Expected behaviour

aiohttp should call custom class passed to aiohttp.web.UrlDispatcher.add_static()

Actual behaviour

response_factory value is ignored

Steps to reproduce

class CORSStreamResponse(web.StreamResponse):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        if 'Access-Control-Allow-Origin' not in self.headers:
            self.headers['Access-Control-Allow-Origin'] = '*'

# ...

app.router.add_static('/static', f'./static', name='static', response_factory=CORSStreamResponse)
$ curl -I http://localhost:8000/static/bundle.js
HTTP/1.1 200 OK
Content-Type: application/javascript
Last-Modified: Tue, 19 Sep 2017 10:08:45 GMT
Content-Length: 84833
Date: Mon, 25 Sep 2017 14:07:59 GMT
Server: Python/3.6 aiohttp/2.2.5

Your environment

aiohttp==2.2.5

@asvetlov
Copy link
Member

asvetlov commented Oct 4, 2017

Not sure if we need support the factory.
Initially it was exist for overriding default behavior.
After aiohttp.web.FileResponse introduction user can make own web-handler for sending custom files easy.

FileResponse is not documented though.
What we really should do is writing docs for the response.

@zemlanin @Axik what is your motivation for passing custom factory instead of using the proposed way?

@zemlanin
Copy link
Author

zemlanin commented Oct 4, 2017

@asvetlov As I wrote in "Steps to reproduce", I've wanted to add Access-Control-Allow-Origin: * header to static file responses (for development)

@asvetlov
Copy link
Member

asvetlov commented Oct 4, 2017

Did you try just setup https://github.com/aio-libs/aiohttp-cors globally for the whole app?

@zemlanin
Copy link
Author

zemlanin commented Oct 4, 2017

What do you mean by "globally for the whole app"? Something like this from aiohttp-cors's usage?

for route in list(app.router.routes()):
    cors.add(route)

If so, then this snippet results in

  File "/usr/local/lib/python3.6/dist-packages/aiohttp_cors/urldispatcher_router_adapter.py", line 331, in add_preflight_handler
    preflight_route = route.resource.add_route(
AttributeError: 'StaticResource' object has no attribute 'add_route'

@Axik
Copy link
Contributor

Axik commented Oct 4, 2017

I'm just a stranger passing by trying to help people.
Anyway we need either fix or remove response_factory argument, because right now it's not working.

@asvetlov
Copy link
Member

asvetlov commented Oct 8, 2017

response_factory dropeed by #2307
The behavior should be supported by aiohttp-cors itself.

@asvetlov asvetlov added this to the 3.0 milestone Oct 19, 2017
@asvetlov asvetlov closed this as completed Feb 9, 2018
@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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

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

No branches or pull requests

3 participants