Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions api/environments/authentication.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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
4 changes: 3 additions & 1 deletion api/environments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()]]
)
Comment on lines +168 to +170
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a temporary fix for now and I think we should perhaps asynchronise this. See further context in the conversation here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO the correct fix would be to only ever use a single cache key for environments, and not have one cache key per server-side SDK key.

To achieve this, the endpoints that accept server-side SDK keys must also receive the client-side key, which is confusing with the current terminology. For example, something like GET /environment-document/abc123 -H "Authorization: ser..." instead of deriving the environment ID from the server-side key.


@hook(AFTER_UPDATE, when="api_key", has_changed=True) # type: ignore[misc]
def update_environment_document_cache(self) -> None:
Expand Down
6 changes: 3 additions & 3 deletions api/environments/sdk/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@
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
from environments.permissions.permissions import EnvironmentKeyPermissions
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

Expand All @@ -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,
)
Expand Down
4 changes: 2 additions & 2 deletions api/tests/unit/environments/test_unit_environments_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -196,23 +198,25 @@ 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())

# 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.clear_environment_cache()
Comment thread
rolodato marked this conversation as resolved.
environment.name = "Updated"
environment.updated_at = timezone.now()
environment.save()

# Then - request with same If-Modified-Since header should return 200
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Loading