feat: implement rate limiting with Redis #174
Conversation
- Add fastapi-ratelimiter and redis dependencies - Create rate limiter configuration with Redis backend - Add test endpoints for rate limiting - Update docker-compose.yml with Redis service - Add Redis configuration to .env.example
WalkthroughThis pull request adds a Redis-backed rate limiting system to the FastAPI backend. It introduces rate limiter configuration, test endpoints demonstrating different rate limit strategies (unlimited, global, and per-user), integrates the middleware into the app startup, adds required dependencies, and updates Docker Compose to include a Redis service. Changes
Sequence DiagramssequenceDiagram
participant Client
participant FastAPI as FastAPI App
participant RateLimitMiddleware
participant Redis
participant Endpoint
Client->>FastAPI: HTTP Request
FastAPI->>RateLimitMiddleware: Process via middleware
RateLimitMiddleware->>RateLimitMiddleware: Extract user identifier<br/>(Bearer token or IP)
RateLimitMiddleware->>Redis: Check/increment quota<br/>for identifier
alt Limit Exceeded
Redis-->>RateLimitMiddleware: RateLimitExceeded
RateLimitMiddleware-->>Client: 429 JSON<br/>(with retry-after)
else Within Limit
Redis-->>RateLimitMiddleware: OK + quota info
RateLimitMiddleware->>Endpoint: Pass request.state<br/>with quota details
Endpoint-->>Client: 200 + response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
backend/test_server.py (1)
10-11: Be cautious about binding the test server to all interfacesUsing
host="0.0.0.0"is fine for local dev or inside Docker, but if someone runs this script directly on a machine it will expose the server on all interfaces. If you ever use this outside a container, consider switching to127.0.0.1or gating the host via an environment variable.As per static analysis hints.
backend/minimal-requirements.txt (1)
1-15: Remove duplicate packages and clarify version strategy vs main requirementsThis minimal requirements file contains several duplicates (
python-multipart,python-jose[cryptography],python-dotenv), which are harmless but add noise. It’d be cleaner to keep each package only once.Also, versions here (e.g.,
fastapi==0.109.2) differ frombackend/requirements.txt(which pins a newer FastAPI). If this file is used for any real runtime or CI image, consider aligning versions or documenting that this is a separate, intentionally different “minimal stack” to avoid subtle incompatibilities.backend/main.py (1)
7-9: Rate limiter setup is correctly wired; confirm that global default limits are desiredCalling
setup_rate_limiter(api)immediately after creating the FastAPI app is a good place to attach middleware and the exception handler. Note that with your currentRateLimiterconfiguration (default_limits=["1000 per day", "100 per hour"]inapp/core/rate_limiter.py), those limits will apply to all routes, in addition to any per‑route@rate_limiter.limitdecorators (depending on library semantics).If you only want explicit endpoints (like the
/testones) to be limited, consider making the global defaults configurable (via settings) or disabling them and relying solely on per‑route limits.Also applies to: 106-110
backend/app/api/v1/test_rate_limit.py (1)
4-29: Gate/testendpoints to non‑production and consider reusing the user ID helperThese endpoints are great for validating the rate limiter, but exposing
/test/*in production could leak implementation details and invite unnecessary probing. Consider only including this router when a debug/development flag is enabled.For
/user-limited, the current key function is effectively per‑IP. If you later move to authenticated users, it’d be cleaner to reuse a shared helper likeget_user_identifier(request)(fromapp.core.rate_limiter) to keep the “who is this request counted against?” logic in one place.backend/app/core/rate_limiter.py (2)
8-15: Redis settings defaults are reasonable; ensure REDIS_HOST is set correctly in DockerThe
RedisSettingsdefaults (localhost:6379, db 0, optional SSL) are fine for local, non-containerized runs. Inside your Docker Compose setup, though, the backend container will needREDIS_HOSTset to the Redis service name (likelyredis), otherwise the default"localhost"will point back to the app container instead of the Redis one.Double‑check that your
.env/ compose environment exportsREDIS_HOST=redis(and the correctREDIS_PORT,REDIS_DB, andREDIS_SSL) so this configuration works consistently across dev and prod.
36-46: Clarifyget_user_identifiersemantics or derive a stable user ID from JWTThe docstring says this extracts a “user identifier,” but the implementation uses the first 8 chars of the raw Bearer token and otherwise falls back to IP. That’s fine for a simple demo, but it won’t give a stable per-user key if tokens rotate, and it isn’t actually decoding any JWT or session.
Either update the docstring to describe the current behavior (token-prefix/IP-based key) or, if you intend true per-user limits, decode the JWT and use a stable claim (e.g.,
suboruser_id) here, then reuse this helper in your routekey_funcs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/api/v1/test_rate_limit.py(1 hunks)backend/app/core/rate_limiter.py(1 hunks)backend/docker-compose.yml(2 hunks)backend/main.py(2 hunks)backend/minimal-requirements.txt(1 hunks)backend/requirements.txt(1 hunks)backend/test_server.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/main.py (1)
backend/app/core/rate_limiter.py (1)
setup_rate_limiter(48-55)
🪛 Ruff (0.14.5)
backend/test_server.py
11-11: Possible binding to all interfaces
(S104)
backend/app/core/rate_limiter.py
25-25: Unused function argument: request
(ARG001)
🔇 Additional comments (2)
backend/requirements.txt (1)
233-236: Redis + fastapi-ratelimiter deps look consistent with the new rate-limiting featureThe new pinned dependencies fit the Redis-backed rate limiter you’ve added. Just make sure every deployment path (Docker image, CI, any “minimal” env using a different requirements file) installs these so imports of
fastapi_ratelimiter/redisnever fail at runtime.backend/app/core/rate_limiter.py (1)
25-35: I'll verify the key claims in this review comment by searching for information about Ruff's ARG001 rule and FastAPI exception handler requirements.Based on my verification of the review comment, the analysis is correct:
Ruff's ARG001 checks for the presence of unused arguments in function definitions. The suggestion to rename the parameter is valid because if a variable is intentionally defined-but-not-used, it should be prefixed with an underscore to adhere to the lint.dummy-variable-rgx pattern. In fact, Ruff does not raise ARG001 when an argument name matches the dummy-variable-rgx pattern, which typically matches names starting with underscore.
For FastAPI specifically, exception handlers receive a Request and the exception, and renaming the unused parameter to
_requestis a standard practice that maintains the required handler signature while satisfying the linter. The code snippet correctly shows that onlyexc.retry_afteris used from the exception object.The refactoring suggestion is sound and follows established conventions.
| redis: | ||
| image: redis:7-alpine | ||
| container_name: redis | ||
| ports: | ||
| - '6379:6379' | ||
| volumes: | ||
| - redis_data:/data | ||
| command: redis-server --requirepass ${REDIS_PASSWORD:-your_secure_password} --appendonly yes | ||
| restart: always | ||
| healthcheck: | ||
| test: ["CMD", "redis-cli", "ping"] | ||
| interval: 5s | ||
| timeout: 30s | ||
| retries: 3 | ||
|
|
There was a problem hiding this comment.
Fix Redis password default to avoid auth mismatch and weak default
The new Redis service wiring and decoupling Falkordb ports look good, but this line is risky:
command: redis-server --requirepass ${REDIS_PASSWORD:-your_secure_password} --appendonly yesIf REDIS_PASSWORD isn’t set, Redis will require your_secure_password while app.core.rate_limiter.redis_settings will try connecting with no password, causing auth failures and leaving an extremely weak default secret in any environment that forgets to set it.
Consider instead requiring the env var explicitly:
- command: redis-server --requirepass ${REDIS_PASSWORD:-your_secure_password} --appendonly yes
+ command: redis-server --requirepass ${REDIS_PASSWORD} --appendonly yesand ensure REDIS_PASSWORD is always set via .env / secrets for both Redis and the backend. Also double‑check that REDIS_HOST for the app is set to the service name redis so connections work correctly in this compose network.
Also applies to: 63-64, 78-79
1 participant
Closes #
📝 Description
🔧 Changes Made
📷 Screenshots or Visual Changes (if applicable)
🤝 Collaboration
Collaborated with:
@username(optional)✅ Checklist
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.