-
Notifications
You must be signed in to change notification settings - Fork 4
CDD-3099 update non public caching #3003
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 all commits
6384f53
c076561
dadfa62
b41d7dd
b76ec72
c66a137
c156a21
b601d14
9f3bc57
999d61c
8684ab6
dde1e9e
834cfda
9a6155b
3334ca0
4ea3e9c
35f78c9
525587a
5bf602d
d71b665
48dcaa8
71567c2
8607fe7
cb19700
95ac503
feb9651
8b8bf22
8de1b86
a229750
affd9a9
37f92e1
a128bed
fea7423
78ac873
3e80b4f
82f45ac
e858469
fcb0de9
749ef2b
9429988
22a4bcd
fc0cd53
96c26cf
fce4ab2
87640b6
25ce032
a5064f6
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 |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| from rest_framework.response import Response | ||
|
|
||
| from caching.private_api.management import CacheManagement, CacheMissError | ||
| from common.auth.cognito_jwt import backend | ||
|
|
||
|
|
||
| class CacheCheckResultedInMissError(Exception): ... | ||
|
|
@@ -14,7 +15,11 @@ def is_caching_v2_enabled() -> bool: | |
| return os.environ.get("CACHING_V2_ENABLED", "").lower() in {"true", "1"} | ||
jeanpierrefouche-ukhsa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def cache_response(*, timeout: int | None = None, is_reserved_namespace: bool = False): | ||
| def cache_response( | ||
| *, | ||
| timeout: int | None = None, | ||
| is_reserved_namespace: bool = False, | ||
| ): | ||
| """Decorator to wrap API views to use a previously cached response. Otherwise, calculate and save on the way out. | ||
|
|
||
| Notes: | ||
|
|
@@ -49,17 +54,33 @@ def cache_response(*, timeout: int | None = None, is_reserved_namespace: bool = | |
| def decorator(view_function): | ||
| @wraps(view_function) | ||
| def wrapped_view(*args, **kwargs) -> Response: | ||
|
|
||
| request = args[1] | ||
| is_public = not (_check_if_valid_non_public_request(request=request)) | ||
|
|
||
| return _retrieve_response_from_cache_or_calculate( | ||
| view_function, timeout, is_reserved_namespace, *args, **kwargs | ||
| view_function, | ||
| timeout, | ||
| is_reserved_namespace, | ||
| is_public, | ||
| *args, | ||
| **kwargs, | ||
| ) | ||
|
|
||
| return wrapped_view | ||
|
|
||
| return decorator | ||
|
|
||
|
|
||
| def _check_if_valid_non_public_request(request) -> bool: | ||
| auth = backend.JSONWebTokenAuthentication() | ||
| result = auth.authenticate(request) | ||
|
|
||
| return result is not None | ||
|
|
||
|
|
||
| def _retrieve_response_from_cache_or_calculate( | ||
| view_function, timeout, is_reserved_namespace, *args, **kwargs | ||
| view_function, timeout, is_reserved_namespace, is_public, *args, **kwargs | ||
| ) -> Response: | ||
| """Gets the response from the cache, otherwise recalculates from the view | ||
|
|
||
|
|
@@ -68,6 +89,10 @@ def _retrieve_response_from_cache_or_calculate( | |
| then the response will always be recalculated from the server | ||
| and no caching will take place. | ||
|
|
||
| If data is not public (i.e. is_public is set to "false"), then the | ||
| response will always be recalculated from the server and no caching | ||
| will take place. | ||
|
|
||
| Args: | ||
| view_function: The view associated with the endpoint | ||
| timeout: The number of seconds after which the response is expired | ||
|
|
@@ -83,9 +108,15 @@ def _retrieve_response_from_cache_or_calculate( | |
|
|
||
| """ | ||
| request: Request = args[1] | ||
| if not is_public: | ||
| return _calculate_response_from_view( | ||
| view_function, *args, is_public=is_public, **kwargs | ||
| ) | ||
|
|
||
| if is_caching_v2_enabled() and not is_reserved_namespace: | ||
| return _calculate_response_from_view(view_function, *args, **kwargs) | ||
| return _calculate_response_from_view( | ||
| view_function, *args, is_public=is_public, **kwargs | ||
| ) | ||
|
|
||
| cache_management = kwargs.pop( | ||
| "cache_management", | ||
|
|
@@ -114,7 +145,9 @@ def _retrieve_response_from_cache_or_calculate( | |
| def _calculate_response_and_save_in_cache( | ||
| view_function, timeout, cache_management, cache_entry_key, *args, **kwargs | ||
| ) -> Response: | ||
| response: Response = _calculate_response_from_view(view_function, *args, **kwargs) | ||
| response: Response = _calculate_response_from_view( | ||
| view_function, *args, is_public=True, **kwargs | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a concern about the assumption that is_public is being derived from authentication context (e.g. JWT presence). It feels like the current logic effectively treats: This may be an oversimplification of the data classification model. Authentication status does not necessarily define whether data is public or private, and conflating these concepts could lead to incorrect caching behaviour or unintended data exposure patterns. More generally, caching decisions (Cache-Control) and data sensitivity classification (public/private) should ideally be derived from the data itself (or an explicit domain-level metadata flag), rather than inferred from request/auth state.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So only non-public requests will have a JWT, all public requests will not. This is the design decision made by the non public team at the outset of this ticket. I agree that the data itself should be checked, but this work is only for updating the caching element, not actually fetching non public data. As it's been agreed that only non-public requests will send a JWT, it was decided that it was the most straightforward approach to determine if the response should be cached or not :) |
||
| ) | ||
| if timeout == 0: | ||
| return response | ||
|
|
||
|
|
@@ -124,5 +157,10 @@ def _calculate_response_and_save_in_cache( | |
| return response | ||
|
|
||
|
|
||
| def _calculate_response_from_view(view_function, *args, **kwargs) -> Response: | ||
| return view_function(*args, **kwargs) | ||
| def _calculate_response_from_view( | ||
| view_function, *args, is_public: bool = True, **kwargs | ||
| ) -> Response: | ||
| response = view_function(*args, **kwargs) | ||
| if not (is_public): | ||
| response["Cache-Control"] = "private, no-cache" | ||
| return response | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| from common.auth.cognito_jwt import backend | ||
| from public_api.metrics_interface.interface import MetricsPublicAPIInterface | ||
| from public_api.version_02.serializers.api_time_series_request_serializer import ( | ||
| APITimeSeriesDTO, | ||
|
|
@@ -39,4 +40,11 @@ def get(self, request: Request, *args, **kwargs) -> Response: | |
| ) | ||
|
|
||
| serializer = self.get_serializer(timeseries_dto_slice, many=True) | ||
| return Response(serializer.data) | ||
| response = Response(data=serializer.data) | ||
|
|
||
| auth = backend.JSONWebTokenAuthentication() | ||
| is_valid_non_public_request = auth.authenticate(request) | ||
| if is_valid_non_public_request: | ||
| response["Cache-Control"] = "private, no-cache" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Evaluate whether Cache-Control: no-store is required here for sensitive/user-specific responses.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting to "private, no-cache" was agreed to after a review of the documentation and ensuring that it aligned with what we are trying to achieve. @luketowell can you confirm if you are happy with "private, no-cache" or if you'd prefer we use "no-store" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider centralising Cache-Control policy strings into a shared constants/module (or system settings if these vary by environment). For example: response["Cache-Control"] = system_settings.CACHE_POLICY_NO_CACHE This makes it easier to audit and update caching policy centrally. |
||
|
|
||
| return response | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| from common.auth.cognito_jwt import backend | ||
| from public_api.metrics_interface.interface import MetricsPublicAPIInterface | ||
| from public_api.serializers.api_time_series_request_serializer import ( | ||
| APITimeSeriesDTO, | ||
|
|
@@ -31,6 +32,7 @@ def _build_request_serializer( | |
|
|
||
| @extend_schema(tags=[PUBLIC_API_TAG]) | ||
| def get(self, request: Request, *args, **kwargs) -> Response: | ||
|
|
||
| serializer: APITimeSeriesRequestSerializer = self._build_request_serializer( | ||
| request=request | ||
| ) | ||
|
|
@@ -39,4 +41,11 @@ def get(self, request: Request, *args, **kwargs) -> Response: | |
| ) | ||
|
|
||
| serializer = self.get_serializer(timeseries_dto_slice, many=True) | ||
| return Response(serializer.data) | ||
| response = Response(data=serializer.data) | ||
|
|
||
| auth = backend.JSONWebTokenAuthentication() | ||
| is_valid_non_public_request = auth.authenticate(request) | ||
| if is_valid_non_public_request: | ||
| response["Cache-Control"] = "private, no-cache" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Derive cache policy string centrally from shared module. system_settings.CACHE_CONTROL_HTTP_HEADER |
||
|
|
||
| return response | ||
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.
Provide justification for this here in the comment.
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 test files have to contain much duplication, as the top base file for both versions of the public api are incredibly similar, meaning that they require incredibly similar tests. These tests are almost full duplicates of each other, but are required to reach the 100% code coverage set. It was agreed with Phill and Josh to exclude these files from the sonarqube duplication check for this reason
Uh oh!
There was an error while loading. Please reload this page.
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.
OK, no probs. Adding it into the code comment on line 13 it will be best, as that allows future readers to see the rationale.