From ac3c281e99beadd1153a54b0eb78a60d23be3e9a Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 24 Apr 2025 12:20:57 +0100 Subject: [PATCH 1/7] Fix test and remove unused request class --- api/environments/authentication.py | 17 ----------------- api/environments/sdk/views.py | 6 +++--- ...t_unit_environments_views_sdk_environment.py | 7 +++++-- 3 files changed, 8 insertions(+), 22 deletions(-) diff --git a/api/environments/authentication.py b/api/environments/authentication.py index 5be62105f907..8fc7d1e0e12b 100644 --- a/api/environments/authentication.py +++ b/api/environments/authentication.py @@ -1,7 +1,6 @@ from common.gunicorn.utils import log_extra from django.conf import settings from django.core.cache import caches -from django.http import HttpRequest from rest_framework.authentication import BaseAuthentication from rest_framework.exceptions import AuthenticationFailed @@ -48,19 +47,3 @@ def authenticate(self, request): # type: ignore[no-untyped-def] # DRF authentication expects a two tuple to be returned containing User, auth return None, None - - -class AuthenticatedRequest(HttpRequest): - _environment: Environment - - @property - def environment(self) -> Environment: - if not self._environment: - raise AssertionError( - "Tried to access environment on an authenticated request, but was None" - ) - return self._environment - - @environment.setter - def environment(self, environment: Environment) -> None: - self._environment = environment diff --git a/api/environments/sdk/views.py b/api/environments/sdk/views.py index 58248d53b12b..ad73b8e3d45f 100644 --- a/api/environments/sdk/views.py +++ b/api/environments/sdk/views.py @@ -4,12 +4,12 @@ from django.utils.decorators import method_decorator from django.views.decorators.http import condition from drf_yasg.utils import swagger_auto_schema # type: ignore[import-untyped] +from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView from core.constants import FLAGSMITH_UPDATED_AT_HEADER from environments.authentication import ( - AuthenticatedRequest, EnvironmentKeyAuthentication, ) from environments.models import Environment @@ -17,7 +17,7 @@ from environments.sdk.schemas import SDKEnvironmentDocumentModel -def get_last_modified(request: AuthenticatedRequest) -> datetime | None: +def get_last_modified(request: Request) -> datetime | None: updated_at: Optional[datetime] = request.environment.updated_at return updated_at @@ -31,7 +31,7 @@ def get_authenticators(self): # type: ignore[no-untyped-def] @swagger_auto_schema(responses={200: SDKEnvironmentDocumentModel}) # type: ignore[misc] @method_decorator(condition(last_modified_func=get_last_modified)) - def get(self, request: AuthenticatedRequest) -> Response: + def get(self, request: Request) -> Response: environment_document = Environment.get_environment_document( request.environment.api_key, ) diff --git a/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py b/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py index 9992e28dec1c..bd83ef7ab97b 100644 --- a/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py +++ b/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py @@ -1,7 +1,9 @@ +import time from typing import TYPE_CHECKING import pytest from django.urls import reverse +from django.utils import timezone from django.utils.http import http_date from flag_engine.segments.constants import EQUAL from rest_framework import status @@ -211,10 +213,11 @@ def test_environment_document_caching( assert response2.status_code == status.HTTP_304_NOT_MODIFIED # When - environment is updated - environment.clear_environment_cache() - environment.name = "Updated" + environment.updated_at = timezone.now() environment.save() + time.sleep(0.1) + # Then - request with same If-Modified-Since header should return 200 response3 = client.get(url) assert response3.status_code == status.HTTP_200_OK From 2b6ed503730ed442141b6d2199e2d0f0a4d4d20e Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 24 Apr 2025 13:23:24 +0100 Subject: [PATCH 2/7] Resolve remaining flakiness --- ...est_unit_environments_views_sdk_environment.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py b/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py index bd83ef7ab97b..857aa85fb945 100644 --- a/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py +++ b/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py @@ -198,26 +198,29 @@ def test_environment_document_caching( # Then - first request should return 200 and include Last-Modified header assert response1.status_code == status.HTTP_200_OK - assert response1.headers["Last-Modified"] == http_date( - environment.updated_at.timestamp() - ) + last_modified = response1.headers["Last-Modified"] + assert last_modified == http_date(environment.updated_at.timestamp()) + + print(f"{last_modified=}") # When - second request with If-Modified-Since header client.credentials( HTTP_X_ENVIRONMENT_KEY=api_key, - HTTP_IF_MODIFIED_SINCE=http_date(environment.updated_at.timestamp()), + HTTP_IF_MODIFIED_SINCE=last_modified, ) response2 = client.get(url) # Then - second request should return 304 Not Modified assert response2.status_code == status.HTTP_304_NOT_MODIFIED + # sleep for 1s since If-Modified-Since is only accurate to the nearest second + # https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/If-Modified-Since + time.sleep(1) + # When - environment is updated environment.updated_at = timezone.now() environment.save() - time.sleep(0.1) - # Then - request with same If-Modified-Since header should return 200 response3 = client.get(url) assert response3.status_code == status.HTTP_200_OK From 82d8e7f86fe33953f038a630088b925007fec776 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 24 Apr 2025 13:30:35 +0100 Subject: [PATCH 3/7] Remove print statement --- .../test_unit_environments_views_sdk_environment.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py b/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py index 857aa85fb945..949bde16a782 100644 --- a/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py +++ b/api/tests/unit/environments/test_unit_environments_views_sdk_environment.py @@ -201,8 +201,6 @@ def test_environment_document_caching( last_modified = response1.headers["Last-Modified"] assert last_modified == http_date(environment.updated_at.timestamp()) - print(f"{last_modified=}") - # When - second request with If-Modified-Since header client.credentials( HTTP_X_ENVIRONMENT_KEY=api_key, From b3fecbee135514ea71257f35f6f43fe84e5b193e Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 24 Apr 2025 14:58:01 +0100 Subject: [PATCH 4/7] Re-add default no cache to test settings --- api/app/settings/test.py | 2 ++ .../identities/test_integration_identities.py | 4 ++-- .../identities/test_unit_identities_views.py | 3 +-- .../test_unit_environments_models.py | 17 +++++++++++++---- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/api/app/settings/test.py b/api/app/settings/test.py index a0018f64dac1..4fd29dc3d1b0 100644 --- a/api/app/settings/test.py +++ b/api/app/settings/test.py @@ -1,6 +1,8 @@ from app.settings.common import * # noqa from app.settings.common import REST_FRAMEWORK +ENVIRONMENT_CACHE_SECONDS = 0 + # We dont want to track tests ENABLE_TELEMETRY = False MAX_PROJECTS_IN_FREE_PLAN = 10 diff --git a/api/tests/integration/environments/identities/test_integration_identities.py b/api/tests/integration/environments/identities/test_integration_identities.py index 48858967b3aa..13c76e2d444b 100644 --- a/api/tests/integration/environments/identities/test_integration_identities.py +++ b/api/tests/integration/environments/identities/test_integration_identities.py @@ -211,8 +211,8 @@ def test_get_feature_states_for_identity_only_makes_one_query_to_get_mv_feature_ variant_2_value, ) - # Then one fewer db queries are made (since the environment is now cached) - with django_assert_num_queries(5): + # Then the same number of queries are made + with django_assert_num_queries(6): second_identity_response = sdk_client.get(url) # Finally, we check that the requests were successful and we got the correct number diff --git a/api/tests/unit/environments/identities/test_unit_identities_views.py b/api/tests/unit/environments/identities/test_unit_identities_views.py index 3691ded0756c..432f8580fb41 100644 --- a/api/tests/unit/environments/identities/test_unit_identities_views.py +++ b/api/tests/unit/environments/identities/test_unit_identities_views.py @@ -964,8 +964,7 @@ def test_get_identities_nplus1( for i in range(2, 13): v1_flag.clone(env=environment, version=i, live_from=now) - # Now it is lower. - with django_assert_num_queries(5): + with django_assert_num_queries(6): api_client.get(url) diff --git a/api/tests/unit/environments/test_unit_environments_models.py b/api/tests/unit/environments/test_unit_environments_models.py index 55c068fcb621..11cbe0e156f0 100644 --- a/api/tests/unit/environments/test_unit_environments_models.py +++ b/api/tests/unit/environments/test_unit_environments_models.py @@ -11,6 +11,7 @@ from mypy_boto3_dynamodb.service_resource import Table from pytest_django import DjangoAssertNumQueries from pytest_django.asserts import assertQuerysetEqual as assert_queryset_equal +from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture from audit.models import AuditLog @@ -200,6 +201,7 @@ def test_environment_clone_clones_multivariate_feature_state_values( def test_environment_get_from_cache_stores_environment_in_cache_on_success( mock_cache: MagicMock, environment: Environment, + settings: SettingsWrapper, ) -> None: # Given mock_cache.get.return_value = None @@ -209,7 +211,9 @@ def test_environment_get_from_cache_stores_environment_in_cache_on_success( # Then assert cached_environment == environment - mock_cache.set.assert_called_with(environment.api_key, environment, timeout=60) + mock_cache.set.assert_called_with( + environment.api_key, environment, timeout=settings.ENVIRONMENT_CACHE_SECONDS + ) def test_environment_get_from_cache_returns_None_if_no_matching_environment( @@ -340,9 +344,14 @@ def test_existence_of_multiple_environment_api_keys_does_not_break_get_from_cach ) -def test_get_from_cache_sets_the_cache_correctly_with_environment_api_key( # type: ignore[no-untyped-def] - environment, environment_api_key, mocker -): +def test_get_from_cache_sets_the_cache_correctly_with_environment_api_key( + environment: Environment, + environment_api_key: EnvironmentAPIKey, + settings: SettingsWrapper, +) -> None: + # Given + settings.ENVIRONMENT_CACHE_SECONDS = 60 + # When returned_environment = Environment.get_from_cache(environment_api_key.key) From 6efb3010a88f8b9f6e2d7dcb1272f5bc04d1e0a9 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 24 Apr 2025 14:58:34 +0100 Subject: [PATCH 5/7] Revert "Re-add default no cache to test settings" This reverts commit b3fecbee135514ea71257f35f6f43fe84e5b193e. --- api/app/settings/test.py | 2 -- .../identities/test_integration_identities.py | 4 ++-- .../identities/test_unit_identities_views.py | 3 ++- .../test_unit_environments_models.py | 17 ++++------------- 4 files changed, 8 insertions(+), 18 deletions(-) diff --git a/api/app/settings/test.py b/api/app/settings/test.py index 4fd29dc3d1b0..a0018f64dac1 100644 --- a/api/app/settings/test.py +++ b/api/app/settings/test.py @@ -1,8 +1,6 @@ from app.settings.common import * # noqa from app.settings.common import REST_FRAMEWORK -ENVIRONMENT_CACHE_SECONDS = 0 - # We dont want to track tests ENABLE_TELEMETRY = False MAX_PROJECTS_IN_FREE_PLAN = 10 diff --git a/api/tests/integration/environments/identities/test_integration_identities.py b/api/tests/integration/environments/identities/test_integration_identities.py index 13c76e2d444b..48858967b3aa 100644 --- a/api/tests/integration/environments/identities/test_integration_identities.py +++ b/api/tests/integration/environments/identities/test_integration_identities.py @@ -211,8 +211,8 @@ def test_get_feature_states_for_identity_only_makes_one_query_to_get_mv_feature_ variant_2_value, ) - # Then the same number of queries are made - with django_assert_num_queries(6): + # Then one fewer db queries are made (since the environment is now cached) + with django_assert_num_queries(5): second_identity_response = sdk_client.get(url) # Finally, we check that the requests were successful and we got the correct number diff --git a/api/tests/unit/environments/identities/test_unit_identities_views.py b/api/tests/unit/environments/identities/test_unit_identities_views.py index 432f8580fb41..3691ded0756c 100644 --- a/api/tests/unit/environments/identities/test_unit_identities_views.py +++ b/api/tests/unit/environments/identities/test_unit_identities_views.py @@ -964,7 +964,8 @@ def test_get_identities_nplus1( for i in range(2, 13): v1_flag.clone(env=environment, version=i, live_from=now) - with django_assert_num_queries(6): + # Now it is lower. + with django_assert_num_queries(5): api_client.get(url) diff --git a/api/tests/unit/environments/test_unit_environments_models.py b/api/tests/unit/environments/test_unit_environments_models.py index 11cbe0e156f0..55c068fcb621 100644 --- a/api/tests/unit/environments/test_unit_environments_models.py +++ b/api/tests/unit/environments/test_unit_environments_models.py @@ -11,7 +11,6 @@ from mypy_boto3_dynamodb.service_resource import Table from pytest_django import DjangoAssertNumQueries from pytest_django.asserts import assertQuerysetEqual as assert_queryset_equal -from pytest_django.fixtures import SettingsWrapper from pytest_mock import MockerFixture from audit.models import AuditLog @@ -201,7 +200,6 @@ def test_environment_clone_clones_multivariate_feature_state_values( def test_environment_get_from_cache_stores_environment_in_cache_on_success( mock_cache: MagicMock, environment: Environment, - settings: SettingsWrapper, ) -> None: # Given mock_cache.get.return_value = None @@ -211,9 +209,7 @@ def test_environment_get_from_cache_stores_environment_in_cache_on_success( # Then assert cached_environment == environment - mock_cache.set.assert_called_with( - environment.api_key, environment, timeout=settings.ENVIRONMENT_CACHE_SECONDS - ) + mock_cache.set.assert_called_with(environment.api_key, environment, timeout=60) def test_environment_get_from_cache_returns_None_if_no_matching_environment( @@ -344,14 +340,9 @@ def test_existence_of_multiple_environment_api_keys_does_not_break_get_from_cach ) -def test_get_from_cache_sets_the_cache_correctly_with_environment_api_key( - environment: Environment, - environment_api_key: EnvironmentAPIKey, - settings: SettingsWrapper, -) -> None: - # Given - settings.ENVIRONMENT_CACHE_SECONDS = 60 - +def test_get_from_cache_sets_the_cache_correctly_with_environment_api_key( # type: ignore[no-untyped-def] + environment, environment_api_key, mocker +): # When returned_environment = Environment.get_from_cache(environment_api_key.key) From cfd456dca586871e00d17225249ca88cac4dc706 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 24 Apr 2025 15:04:05 +0100 Subject: [PATCH 6/7] Fix environment cache clear --- api/environments/models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/environments/models.py b/api/environments/models.py index d872645c6d14..17eb27999ca3 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -165,7 +165,9 @@ def create_feature_states(self) -> None: @hook(AFTER_UPDATE) # type: ignore[misc] def clear_environment_cache(self) -> None: # TODO: this could rebuild the cache itself (using an async task) - environment_cache.delete(self.initial_value("api_key")) + environment_cache.delete_many( + [self.initial_value("api_key"), *[eak.key for eak in self.api_keys.all()]] + ) @hook(AFTER_UPDATE, when="api_key", has_changed=True) # type: ignore[misc] def update_environment_document_cache(self) -> None: From a9d9d39b49900fd02abcc4d2b7a31430098f9841 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 24 Apr 2025 15:10:23 +0100 Subject: [PATCH 7/7] Fix regressions --- api/tests/unit/environments/test_unit_environments_models.py | 4 ++-- .../unit/integrations/amplitude/test_unit_amplitude_models.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/tests/unit/environments/test_unit_environments_models.py b/api/tests/unit/environments/test_unit_environments_models.py index 55c068fcb621..0aa5abb336cc 100644 --- a/api/tests/unit/environments/test_unit_environments_models.py +++ b/api/tests/unit/environments/test_unit_environments_models.py @@ -408,9 +408,9 @@ def test_save_environment_clears_environment_cache(mocker, project): # type: ig environment.save() # Then - mock_calls = mock_environment_cache.delete.mock_calls + mock_calls = mock_environment_cache.delete_many.mock_calls assert len(mock_calls) == 2 - assert mock_calls[0][1][0] == mock_calls[1][1][0] == old_key + assert mock_calls[0][1][0] == mock_calls[1][1][0] == [old_key] @pytest.mark.parametrize( diff --git a/api/tests/unit/integrations/amplitude/test_unit_amplitude_models.py b/api/tests/unit/integrations/amplitude/test_unit_amplitude_models.py index 86c6214ab42b..e858319e4a0a 100644 --- a/api/tests/unit/integrations/amplitude/test_unit_amplitude_models.py +++ b/api/tests/unit/integrations/amplitude/test_unit_amplitude_models.py @@ -60,4 +60,4 @@ def test_amplitude_configuration_update_clears_environment_cache(environment, mo amplitude_config.save() # Then - mock_environment_cache.delete.assert_called_once_with(environment.api_key) + mock_environment_cache.delete_many.assert_called_once_with([environment.api_key])