-
Notifications
You must be signed in to change notification settings - Fork 516
feat: Add rate limiting to identity search endpoint #6438
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
eb6c827
db1cc77
14dc93f
aa244d5
a11a21e
c207b15
b17cd1a
61b5b7e
761d0a8
7dbb0dc
ba7ab9c
4d4dced
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |||||
| from rest_framework import status, viewsets | ||||||
| from rest_framework.permissions import IsAuthenticated | ||||||
| from rest_framework.response import Response | ||||||
| from rest_framework.throttling import ScopedRateThrottle | ||||||
|
|
||||||
| from app.pagination import CustomPagination | ||||||
| from core.constants import FLAGSMITH_UPDATED_AT_HEADER, SDK_ENVIRONMENT_KEY_HEADER | ||||||
|
|
@@ -41,6 +42,15 @@ | |||||
| class IdentityViewSet(viewsets.ModelViewSet): # type: ignore[type-arg] | ||||||
| serializer_class = IdentitySerializer | ||||||
| pagination_class = CustomPagination | ||||||
| throttle_scope = "identity_search" | ||||||
|
cursor[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
|
1-23-smy marked this conversation as resolved.
|
||||||
| def get_throttles(self): # type: ignore[no-untyped-def] | ||||||
| """ | ||||||
| Apply identity_search throttle only to list (search) requests. | ||||||
| """ | ||||||
| if getattr(self, "action", None) == "list": | ||||||
|
||||||
| if getattr(self, "action", None) == "list": | |
| if getattr(self, "action", None) == "list" and "q" in self.request.query_params: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the impact is minimal, i would suggest to return the global default throttle super().get_throttles here (defaulting to DEFAULT_THROTTLE_CLASSES). Similarly to what we did here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I have reverted debounce from 750ms back to 500ms (agreed UX time).
- Updated get_throttles() to return super().get_throttles() for non-list actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot! Just some conflicts to fix and we should be good to enable the workflows and get it over the line. Again, thanks for the contribution, we appreciate!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! the conflicts should now be resolved.
Let me know if there's anything else you'd like me to adjust!
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -305,6 +305,31 @@ def test_search_identities_still_allows_paging( | |
| assert response2.data["results"] | ||
|
|
||
|
|
||
| def test_identity_search_is_throttled( | ||
| admin_client: APIClient, | ||
| environment: Environment, | ||
| reset_cache: None, | ||
| mocker: MockerFixture, | ||
| ) -> None: | ||
| # Given - mock the throttle rate to be restrictive for testing | ||
| mocker.patch( | ||
| "rest_framework.throttling.ScopedRateThrottle.get_rate", return_value="1/minute" | ||
| ) | ||
| base_url = reverse( | ||
| "api-v1:environments:environment-identities-list", | ||
| args=[environment.api_key], | ||
| ) | ||
| url = f"{base_url}?q=test" | ||
|
|
||
| # When - make 2 requests in quick succession | ||
| response1 = admin_client.get(url) | ||
| response2 = admin_client.get(url) | ||
|
|
||
| # Then - first should succeed, second should be throttled | ||
| assert response1.status_code == status.HTTP_200_OK | ||
| assert response2.status_code == status.HTTP_429_TOO_MANY_REQUESTS | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
|
Comment on lines
+308
to
+331
|
||
|
|
||
| def test_can_delete_identity( | ||
| environment: Environment, | ||
| admin_client: APIClient, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,10 @@ import useDebounce from './useDebounce' | |
| export default function useDebouncedSearch(initialValue = '') { | ||
| const [searchInput, setSearchInput] = useState(initialValue) | ||
| const [search, setSearch] = useState(initialValue) | ||
| const [debounceTime, setDebounceTime] = useState(500) | ||
| const [debounceTime, setDebounceTime] = useState(750) | ||
|
|
||
| useEffect(() => { | ||
| setDebounceTime(searchInput.length < 1 ? 0 : 500) | ||
| setDebounceTime(searchInput.length < 1 ? 0 : 750) | ||
|
||
| }, [searchInput]) | ||
|
|
||
| const debouncedSearch = useDebounce((value: string) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description mentions "Frontend Changes" that increase debounce from 500ms to 750ms in
frontend/common/useDebouncedSearch.ts, but this file is not included in the current changes. Based on previous review feedback, there were concerns about this change affecting all components using the hook, and it was suggested to stick to 500ms if there's no conflict with the backend throttling. Either include the frontend changes in this PR or remove the mention from the PR description.