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

[Bug] Dependency injection not working for websocket handlers #61

Closed
xmcp opened this issue Feb 14, 2022 · 1 comment · Fixed by #64
Closed

[Bug] Dependency injection not working for websocket handlers #61

xmcp opened this issue Feb 14, 2022 · 1 comment · Fixed by #64

Comments

@xmcp
Copy link

xmcp commented Feb 14, 2022

Describe the bug

Sanic-Ext fails to inject dependencies to @app.websocket() handlers.

Screenshots

image

To Reproduce

Try this code:

from sanic import Sanic

app = Sanic('test')

class Foo:
    pass
    
app.ext.add_dependency(Foo, lambda req: None)

@app.websocket('/foo')
async def handler(req, ws, foo: Foo):
    pass
    
app.run(port=4321, debug=True)

Visit http://127.0.0.1:4321/, then execute new WebSocket('ws://127.0.0.1:4321/foo') in browser console.

This error will be thrown:

Traceback (most recent call last):
  File "handle_request", line 83, in handle_request
    )
  File "C:\Users\xmcp\AppData\Roaming\Python\Python38\site-packages\sanic\app.py", line 1002, in _websocket_handler
    fut = ensure_future(handler(request, ws, *args, **kwargs))
TypeError: handler() missing 1 required positional argument: 'foo'

Expected behavior

It should not throw an error.

Environment (please complete the following information):

  • OS: Windows 10 (19044.1466)
  • Browser: not related
  • Version: sanic 21.12.1, sanic-ext 22.1.2

Additional context

Sanic wraps a functools.partial to the websocket handler, effectively erasing the type signature of the handler function. Therefore sanic_ext.extensions.injection.injector (screenshot below) cannot get its type hint:

image

@ahopkins
Copy link
Member

Thanks @xmcp for bringing this up. We will have this fixed in the upcoming release next week.

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

Successfully merging a pull request may close this issue.

2 participants