-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: [FC-0092] Optimize Course Info Blocks API #37122
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
Changes from 17 commits
3a0ab4b
24e7bfe
92d50b9
86c42f3
1845775
2fc81d4
5bc490e
7ef8fd1
b40d755
76b9cb3
a25a123
9e0d736
e2ad6ec
e6d8fc1
3ec750c
2a0b398
f7bc539
74bb27f
37e7a9e
9565fc8
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 |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| """ | ||
| API function for retrieving course blocks data | ||
| """ | ||
|
|
||
| from edx_django_utils.cache import RequestCache | ||
|
|
||
| import lms.djangoapps.course_blocks.api as course_blocks_api | ||
| from lms.djangoapps.course_blocks.transformers.access_denied_filter import AccessDeniedMessageFilterTransformer | ||
|
|
@@ -14,6 +14,7 @@ | |
| from .toggles import HIDE_ACCESS_DENIALS_FLAG | ||
| from .transformers.blocks_api import BlocksAPITransformer | ||
| from .transformers.milestones import MilestonesAndSpecialExamsTransformer | ||
| from .utils import UNFILTERED_STRUCTURE_CACHE_KEY, REUSABLE_BLOCKS_CACHE_KEY | ||
|
|
||
|
|
||
| def get_blocks( | ||
|
|
@@ -29,6 +30,7 @@ def get_blocks( | |
| block_types_filter=None, | ||
| hide_access_denials=False, | ||
| allow_start_dates_in_future=False, | ||
| cache_with_future_dates=False, | ||
| ): | ||
| """ | ||
| Return a serialized representation of the course blocks. | ||
|
|
@@ -61,6 +63,7 @@ def get_blocks( | |
| allow_start_dates_in_future (bool): When True, will allow blocks to be | ||
| returned that can bypass the StartDateTransformer's filter to show | ||
| blocks with start dates in the future. | ||
| cache_with_future_dates (bool): When True, will use the block caching logic using RequestCache | ||
| """ | ||
|
|
||
| if HIDE_ACCESS_DENIALS_FLAG.is_enabled(): | ||
|
|
@@ -118,6 +121,10 @@ def get_blocks( | |
| ), | ||
| ] | ||
|
|
||
| if cache_with_future_dates: | ||
| # Include future dates such that get_course_assignments can reuse the block structure from RequestCache | ||
| allow_start_dates_in_future = True | ||
|
|
||
| # transform | ||
| blocks = course_blocks_api.get_course_blocks( | ||
| user, | ||
|
|
@@ -128,6 +135,19 @@ def get_blocks( | |
| include_has_scheduled_content=include_has_scheduled_content | ||
| ) | ||
|
|
||
| if cache_with_future_dates: | ||
| # Store a copy of the transformed, but still unfiltered, course blocks in RequestCache to be reused | ||
| # wherever possible for optimization. Copying is required to make sure the cached structure is not mutated | ||
| # by the filtering below. | ||
| request_cache = RequestCache(UNFILTERED_STRUCTURE_CACHE_KEY) | ||
| request_cache.set(REUSABLE_BLOCKS_CACHE_KEY, blocks.copy()) | ||
|
Contributor
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. We're doing a copy on the way in here, but we also have to worry about people mutating what they get back from
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. That is a reasonable concern, but once again, we need to take into account whether this affects performance. And unfortunately it does:
Contributor
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. If it's going to severely impact performance, then it's okay to have those kinds of side-effects, though we should put comments in the docstring of
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. Yes, I added a word of caution to the function's docstring. |
||
|
|
||
| # Since we included blocks with future start dates in our block structure, | ||
| # we need to include the 'start' field to filter out such blocks before returning the response. | ||
| # If 'start' field is not requested, it will be removed from the response. | ||
| requested_fields = set(requested_fields) | ||
| requested_fields.add('start') | ||
|
|
||
| # filter blocks by types | ||
| if block_types_filter: | ||
| block_keys_to_remove = [] | ||
|
|
@@ -142,7 +162,7 @@ def get_blocks( | |
| serializer_context = { | ||
| 'request': request, | ||
| 'block_structure': blocks, | ||
| 'requested_fields': requested_fields or [], | ||
| 'requested_fields': requested_fields, | ||
| } | ||
|
|
||
| if return_type == 'dict': | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -237,6 +237,7 @@ def list(self, request, usage_key_string, hide_access_denials=False): # pylint: | |
| params.cleaned_data['return_type'], | ||
| params.cleaned_data.get('block_types_filter', None), | ||
| hide_access_denials=hide_access_denials, | ||
| cache_with_future_dates=True | ||
| ) | ||
| ) | ||
| # If the username is an empty string, and not None, then we are requesting | ||
|
|
@@ -339,9 +340,52 @@ def list(self, request, hide_access_denials=False): # pylint: disable=arguments | |
| if not root: | ||
| raise ValidationError(f"Unable to find course block in '{course_key_string}'") | ||
|
|
||
| # Earlier we included blocks with future start dates in the collected/cached block structure. | ||
| # Now we need to emulate allow_start_dates_in_future=False by removing any such blocks. | ||
| include_start = "start" in request.query_params['requested_fields'] | ||
| self.remove_future_blocks(course_blocks, include_start) | ||
|
|
||
| recurse_mark_complete(root, course_blocks) | ||
| return response | ||
|
|
||
| @staticmethod | ||
| def remove_future_blocks(course_blocks, include_start: bool): | ||
| """ | ||
| Mutates course_blocks in place: | ||
| - removes blocks whose 'start' is in the future | ||
| - also removes references to them from parents' 'children' lists | ||
| - removes 'start' key from all blocks if it wasn't requested | ||
| """ | ||
| from datetime import datetime, timezone | ||
|
Contributor
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. There's no need for this to be a function-local import, is there?
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. There isn't indeed, moved the import to module level |
||
|
|
||
| if not course_blocks: | ||
| return course_blocks | ||
|
|
||
| now = datetime.now(timezone.utc) | ||
|
|
||
| # 1. Collect IDs of blocks to remove | ||
| to_remove = set() | ||
| for block_id, block in course_blocks.items(): | ||
| get_field = block.get if include_start else block.pop | ||
| start = get_field("start") | ||
| if start and start > now: | ||
| to_remove.add(block_id) | ||
|
Comment on lines
+370
to
+371
Contributor
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. It seems like this function is ignoring
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. At this point, we are dealing with the start dates that have been computed by |
||
|
|
||
| if not to_remove: | ||
| return course_blocks | ||
|
|
||
| # 2. Remove the blocks themselves | ||
| for block_id in to_remove: | ||
| course_blocks.pop(block_id, None) | ||
|
|
||
| # 3. Clean up children lists | ||
| for block in course_blocks.values(): | ||
| children = block.get("children") | ||
| if children: | ||
| block["children"] = [cid for cid in children if cid not in to_remove] | ||
|
Comment on lines
+381
to
+384
Contributor
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. Just to check my understanding: Is it the case that it's okay to do this simple child removal (and not go down into further descendants) because all the inheritance has already been pre-computed, and the
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. Yes, this is correct: by this point, |
||
|
|
||
| return course_blocks | ||
|
|
||
|
|
||
| @method_decorator(transaction.non_atomic_requests, name='dispatch') | ||
| @view_auth_classes(is_authenticated=False) | ||
|
|
||
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.
Nit: The naming of this is confusing because
UNFILTERED_STRUCTURE_CACHE_KEYimplies that this is a cache key as part of a key-value pairing, butUNFILTERED_STRUCTURE_CACHE_KEYis really a namespace for this particular RequestCache as a whole. The namespaces just ensure that there's no chance of collision with other apps that need RequestCache functionality, so it would be more common to make the namespace be the module or app name.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.
Good point, changed it to
COURSE_API_REQUEST_CACHE_NAMESPACE = "course_api"