🐛 Fix support for path parameters in WebSockets

[davidbrochart]

Because a path can look like /files/{file_path:path}, we need to strip the type (e.g. :path) of the parameter.

[adam-tokarski]

Care to add some tests for that?

[davidbrochart]

Care to add some tests for that?

Sure, let me know what you think.

[adam-tokarski]

Generally LGTM, but personally I'd

• add more tests for not so happy paths, e.g. "/path/{param::}" or "/path:smth/{param}"
• introduce kind of parametrized (if available here), but it would not change too much

[davidbrochart]

• add more tests for not so happy paths, e.g. "/path/{param::}" or "/path:smth/{param}"

Done.

• introduce kind of parametrized (if available here), but it would not change too much

parametrized is not used in FastAPI yet, and it seems a bit overkill to me, so I didn't use it.

[davidbrochart]

@adam-tokarski do you think this can be merged?

[adam-tokarski]

@adam-tokarski do you think this can be merged?

If there are not any edge cases for which it could break anything I would say - yes, why not.

[davidbrochart]

I don't see any edge cases. What's the next step to have this PR in?

[ghandic]

The only person that can merge is @tiangolo he'll take a look at it soon

[davidbrochart]

@tiangolo do you think this can go in? We are currently monkey-patching the change in Jupyter.

[codecov]

Codecov Report

Merging #3879 (aa48787) into master (0bb8920) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #3879   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          535       535           
  Lines        13825     13838   +13     
=========================================
+ Hits         13825     13838   +13     

Impacted Files	Coverage Δ
fastapi/routing.py	100.00% <100.00%> (ø)
tests/test_ws_router.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[github-actions]

📝 Docs preview for commit 6552462 at: https://628205635239266587b6e591--fastapi.netlify.app

[davidbrochart]

The Jupyter server still needs to monkey-patch FastAPI, is there any chance this change could go in?

[Kludex]

What's this for?

[davidbrochart]

Thanks for chiming in @Kludex!
This is to support parameter types in WebSocket endpoints, for instance:

@app.websocket("/files/{file_path:path}")
async def websocket_endpoint(websocket: WebSocket, file_path):
    ...

[Kludex]

I'll check later, but if you can answer "why this is not needed on the HTTP methods?" would be helpful.

[Kludex]

Also, just to set the expectations... I don't have merge rights. 👀

[davidbrochart]

Also, just to set the expectations... I don't have merge rights. eyes

No worries, it's already great to have your feedback.

I'll check later, but if you can answer "why this is not needed on the HTTP methods?" would be helpful.

Your question made me revisit the issue, and it turns out that there is a better fix.
The APIWebSocketRoute was not treated the same way as the APIRoute, as far as their dependant path is concerned. Passing self.path_format for the former, as it is done for the latter, fixes the issue in a much nicer way.

[github-actions]

📝 Docs preview for commit f1afa0d at: https://630f2f2d4b042e7bbc6f2c70--fastapi.netlify.app

[Kludex]

Now it makes more sense. 👍

A test checking that query and path params can be passed as expected might be a good idea. 👀

[davidbrochart]

A test checking that query and path params can be passed as expected might be a good idea.

👍
Without the fix, the test fails as expected:

tests/test_ws_router.py::test_router_with_params FAILED                                                                                       [100%]

===================================================================== FAILURES ======================================================================
______________________________________________________________ test_router_with_params ______________________________________________________________

    def test_router_with_params():
        client = TestClient(app)
>       with client.websocket_connect("/router/path/to/file?queryparam=a_query_param") as websocket:

tests/test_ws_router.py:120: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../../mambaforge/envs/fastapi/lib/python3.10/site-packages/starlette/testclient.py:316: in __enter__
    self._raise_on_close(message)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <starlette.testclient.WebSocketTestSession object at 0x7fba1ed72ef0>, message = {'code': 1008, 'reason': '', 'type': 'websocket.close'}

    def _raise_on_close(self, message: Message) -> None:
        if message["type"] == "websocket.close":
>           raise WebSocketDisconnect(
                message.get("code", 1000), message.get("reason", "")
            )
E           starlette.websockets.WebSocketDisconnect: (1008, '')

../../../mambaforge/envs/fastapi/lib/python3.10/site-packages/starlette/testclient.py:357: WebSocketDisconnect

[github-actions]

📝 Docs preview for commit 7c7c76e at: https://630f5c1603e32307504c07f3--fastapi.netlify.app

[Kludex]

This makes sense. 👍

Thanks @davidbrochart 🙏

[tiangolo]

Awesome, thank you @davidbrochart! 🤓

It's so exciting you are using FastAPI in some part of Jupyter!

Also thanks everyone for the help here! 🚀

This will be available in the next release, in some hours (the current is 0.81.0). 🎉