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

feat: support anyio, sending denial response, handshake headers #34

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

WSH032
Copy link
Owner

@WSH032 WSH032 commented Apr 7, 2024

Summary

Added

Changed

Test

Checklist

  • I've read CONTRIBUTING.md.
  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • 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.

Copy link

codecov bot commented Apr 7, 2024

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 ⚠️
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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WSH032
Copy link
Owner Author

WSH032 commented Apr 8, 2024

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

@WSH032 WSH032 added the help wanted Extra attention is needed label Apr 8, 2024
@WSH032
Copy link
Owner Author

WSH032 commented Apr 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant