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

Fix rate limit bug on Websockets #1712

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Fix rate limit bug on Websockets #1712

merged 1 commit into from
Dec 18, 2024

Conversation

NolanTrem
Copy link
Collaborator

@NolanTrem NolanTrem commented Dec 18, 2024

Important

Fixes rate limit bug on Websockets by adding websocket_rate_limit_dependency and updating route dependencies in base_router.py and logs_router.py.

  • Behavior:
    • Adds websocket_rate_limit_dependency in base_router.py to handle rate limiting for WebSocket connections.
    • Updates WebSocket route in logs_router.py to use websocket_rate_limit_dependency instead of rate_limit_dependency.
  • Functions:
    • Introduces websocket_rate_limit_dependency in BaseRouterV3 class in base_router.py.
    • Modifies _setup_routes() in LogsRouter class in logs_router.py to apply the new dependency.
  • Misc:
    • Minor import change in base_router.py to include WebSocket from fastapi.

This description was created by Ellipsis for 43de996. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 43de996 in 1 minute and 53 seconds

More details
  • Looked at 43 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_pJq42I4bZKfjNVAV


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -206,4 +206,15 @@ async def rate_limit_dependency(
user_id, route
)

async def websocket_rate_limit_dependency(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The websocket_rate_limit_dependency function does not implement rate limiting logic. It should include logic to check and enforce rate limits, similar to rate_limit_dependency.

@NolanTrem NolanTrem merged commit f26e93b into main Dec 18, 2024
13 of 14 checks passed
@NolanTrem NolanTrem deleted the Nolan/LogsBug branch December 18, 2024 22:20
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 this pull request may close these issues.

1 participant