fastapi-proxy-lib icon indicating copy to clipboard operation
fastapi-proxy-lib copied to clipboard

feat: support `anyio`, sending denial response, handshake headers

Open WSH032 opened this issue 1 year ago • 3 comments

Summary

Added

Changed

Test

Checklist

  • [x] I've read CONTRIBUTING.md.
  • [x] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • [x] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • [ ] I've updated the documentation accordingly.

WSH032 avatar Apr 07 '24 00:04 WSH032

Codecov Report

Attention: Patch coverage is 85.41667% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 96.39%. Comparing base (977d9c1) to head (3d2b0ea).

Files Patch % Lines
src/fastapi_proxy_lib/core/websocket.py 84.09% 10 Missing and 4 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #34      +/-   ##
==========================================
- Coverage   96.74%   96.39%   -0.35%     
==========================================
  Files           9        9              
  Lines         461      472      +11     
  Branches       67       73       +6     
==========================================
+ Hits          446      455       +9     
- Misses          9       11       +2     
  Partials        6        6              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 07 '24 00:04 codecov[bot]

The test succeeded on Windows but failed on Linux, preventing us from merging. Unfortunately, I don't have time to address this issue at the moment. If someone is willing to pick it up, I can provide guidance.

Need Improvement

  • Revert: feat!: remove path parameter in proxy method The downstream is already passing this parameter. Let's issue a deprecation warning in this version and remove it in the next version.
  • Revert: the return signature of ReverseWebSocketProxy.proxy has been changed in this commit Let's keep the original signature so users can handle responses themselves; we should provide a helper method to assist users in sending denial responses rather than sending responses ourselves.

TODO

  • Use ... to represent default parameters instead of importing these private variables from httpx-ws

    https://github.com/WSH032/fastapi-proxy-lib/blob/3d2b0ea27a0b083886c1d03e37645bb5a7050dfa/src/fastapi_proxy_lib/core/websocket.py#L39-L44

  • Fix: can not receive the websocket close reason

    https://github.com/WSH032/fastapi-proxy-lib/blob/3d2b0ea27a0b083886c1d03e37645bb5a7050dfa/tests/test_ws.py#L212-L215 This is an upstream issue of ASGI, and we are unable to address it. We will track it here frankie567/httpx-ws#70.

  • Fix: unable to read the content of WebSocketUpgradeError.response

    https://github.com/WSH032/fastapi-proxy-lib/blob/3d2b0ea27a0b083886c1d03e37645bb5a7050dfa/src/fastapi_proxy_lib/core/websocket.py#L440-L442 This is an upstream issue of httpx-ws, We will track it here frankie567/httpx-ws#69

  • Use StreamingResponse in favor of Response

    https://github.com/WSH032/fastapi-proxy-lib/blob/3d2b0ea27a0b083886c1d03e37645bb5a7050dfa/src/fastapi_proxy_lib/core/websocket.py#L431-L438 This is an upstream issue of starlette, We will track it here encode/starlette#2566

WSH032 avatar Apr 08 '24 15:04 WSH032

I am aware of such downstream use cases, so we cannot deprecate the path parameter, which can allow us to add a dynamic path prefix.

WSH032 avatar Apr 11 '24 03:04 WSH032