From 700a902b68dc07ad34a2a0fe761c9401cefde7bd Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri <nasthagiri@edx.org> Date: Thu, 30 Aug 2018 14:32:29 -0400 Subject: [PATCH] Cleanup and remove deprecated RequestCache Django app ARCH-223 --- .../contentstore/api/views/course_quality.py | 42 +-- cms/djangoapps/xblock_config/models.py | 4 +- common/djangoapps/course_modes/models.py | 4 +- .../djangoapps/django_comment_common/utils.py | 4 +- common/djangoapps/edxmako/paths.py | 4 +- common/djangoapps/edxmako/request_context.py | 6 +- common/djangoapps/student/models.py | 8 +- common/djangoapps/student/roles.py | 2 +- common/djangoapps/student/tests/test_email.py | 2 +- common/djangoapps/track/contexts.py | 2 +- .../track/event_transaction_utils.py | 2 +- common/djangoapps/util/db.py | 2 +- common/djangoapps/util/milestones_helpers.py | 2 +- .../xmodule/modulestore/mongo/draft.py | 16 +- .../xmodule/partitions/partitions_service.py | 4 +- .../xmodule/video_module/video_module.py | 15 +- lms/djangoapps/ccx/overrides.py | 2 +- lms/djangoapps/course_wiki/middleware.py | 2 +- .../courseware/context_processor.py | 2 +- lms/djangoapps/courseware/middleware.py | 2 +- .../courseware/tests/test_video_mongo.py | 3 +- .../discussion/tests/test_signals.py | 4 +- .../django_comment_client/permissions.py | 4 +- lms/djangoapps/django_comment_client/utils.py | 10 +- lms/djangoapps/grades/config/models.py | 4 +- lms/djangoapps/grades/models.py | 2 +- lms/djangoapps/grades/scores.py | 4 +- lms/djangoapps/mobile_api/middleware.py | 2 +- lms/djangoapps/teams/search_indexes.py | 2 +- .../block_structure/config/__init__.py | 6 +- .../block_structure/transformer_registry.py | 4 +- .../core/djangoapps/course_groups/cohorts.py | 14 +- openedx/core/djangoapps/credit/models.py | 4 +- openedx/core/djangoapps/embargo/middleware.py | 2 +- .../core/djangoapps/oauth_dispatch/models.py | 2 +- .../core/djangoapps/request_cache/__init__.py | 83 ----- .../djangoapps/request_cache/middleware.py | 82 ----- .../core/djangoapps/request_cache/tests.py | 250 -------------- .../schedules/content_highlights.py | 2 +- openedx/core/djangoapps/theming/helpers.py | 4 +- .../djangoapps/user_api/course_tag/api.py | 2 +- openedx/core/djangoapps/util/signals.py | 16 +- .../djangoapps/util/tests/test_signals.py | 24 ++ .../djangoapps/util/tests/test_user_utils.py | 2 +- .../verified_track_content/models.py | 4 +- .../core/djangoapps/waffle_utils/__init__.py | 2 +- .../core/djangoapps/waffle_utils/models.py | 4 +- openedx/core/lib/cache_utils.py | 144 ++++++--- openedx/core/lib/celery/task_utils.py | 4 +- openedx/core/lib/plugins.py | 4 +- .../core/lib/request_utils.py | 35 +- openedx/core/lib/tests/test_cache_utils.py | 304 +++++++++++++++--- .../core/lib/tests/test_request_utils.py | 23 +- openedx/features/course_experience/utils.py | 4 +- 54 files changed, 591 insertions(+), 596 deletions(-) delete mode 100644 openedx/core/djangoapps/request_cache/__init__.py delete mode 100644 openedx/core/djangoapps/request_cache/middleware.py delete mode 100644 openedx/core/djangoapps/request_cache/tests.py create mode 100644 openedx/core/djangoapps/util/tests/test_signals.py rename common/djangoapps/util/request.py => openedx/core/lib/request_utils.py (57%) rename common/djangoapps/util/tests/test_request.py => openedx/core/lib/tests/test_request_utils.py (81%) diff --git a/cms/djangoapps/contentstore/api/views/course_quality.py b/cms/djangoapps/contentstore/api/views/course_quality.py index 4bff8e3c2dc..434c0f993fa 100644 --- a/cms/djangoapps/contentstore/api/views/course_quality.py +++ b/cms/djangoapps/contentstore/api/views/course_quality.py @@ -7,7 +7,7 @@ from rest_framework.response import Response from contentstore.views.item import highlights_setting from edxval.api import get_videos_for_course -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from openedx.core.lib.graph_traversals import traverse_pre_order from xmodule.modulestore.django import modulestore @@ -172,26 +172,27 @@ class CourseQualityView(DeveloperErrorViewMixin, GenericAPIView): durations=self._stats_dict(video_durations), ) - @request_cached - def _get_subsections_and_units(self, course, request): + @classmethod + @request_cached() + def _get_subsections_and_units(cls, course, request): """ Returns {subsection_key: {unit_key: {num_leaf_blocks: <>, leaf_block_types: set(<>) }}} for all visible subsections and units. """ - _, visible_sections = self._get_sections(course) + _, visible_sections = cls._get_sections(course) subsection_dict = {} for section in visible_sections: - visible_subsections = self._get_visible_children(section) + visible_subsections = cls._get_visible_children(section) if get_bool_param(request, 'exclude_graded', False): visible_subsections = [s for s in visible_subsections if not s.graded] for subsection in visible_subsections: unit_dict = {} - visible_units = self._get_visible_children(subsection) + visible_units = cls._get_visible_children(subsection) for unit in visible_units: - leaf_blocks = self._get_leaf_blocks(unit) + leaf_blocks = cls._get_leaf_blocks(unit) unit_dict[unit.location] = dict( num_leaf_blocks=len(leaf_blocks), leaf_block_types=set(block.location.block_type for block in leaf_blocks), @@ -200,39 +201,44 @@ class CourseQualityView(DeveloperErrorViewMixin, GenericAPIView): subsection_dict[subsection.location] = unit_dict return subsection_dict - @request_cached - def _get_sections(self, course): - return self._get_all_children(course) + @classmethod + @request_cached() + def _get_sections(cls, course): + return cls._get_all_children(course) - def _get_all_children(self, parent): + @classmethod + def _get_all_children(cls, parent): store = modulestore() - children = [store.get_item(child_usage_key) for child_usage_key in self._get_children(parent)] + children = [store.get_item(child_usage_key) for child_usage_key in cls._get_children(parent)] visible_children = [ c for c in children if not c.visible_to_staff_only and not c.hide_from_toc ] return children, visible_children - def _get_visible_children(self, parent): - _, visible_chidren = self._get_all_children(parent) + @classmethod + def _get_visible_children(cls, parent): + _, visible_chidren = cls._get_all_children(parent) return visible_chidren - def _get_children(self, parent): + @classmethod + def _get_children(cls, parent): if not hasattr(parent, 'children'): return [] else: return parent.children - def _get_leaf_blocks(self, unit): + @classmethod + def _get_leaf_blocks(cls, unit): def leaf_filter(block): return ( block.location.block_type not in ('chapter', 'sequential', 'vertical') and - len(self._get_children(block)) == 0 + len(cls._get_children(block)) == 0 ) return [ block for block in - traverse_pre_order(unit, self._get_visible_children, leaf_filter) + traverse_pre_order(unit, cls._get_visible_children, leaf_filter) ] def _stats_dict(self, data): diff --git a/cms/djangoapps/xblock_config/models.py b/cms/djangoapps/xblock_config/models.py index 8093e40f5b7..a2b46fb69da 100644 --- a/cms/djangoapps/xblock_config/models.py +++ b/cms/djangoapps/xblock_config/models.py @@ -9,7 +9,7 @@ from config_models.models import ConfigurationModel from django.db.models import TextField from opaque_keys.edx.django.models import CourseKeyField -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached class StudioConfig(ConfigurationModel): @@ -42,7 +42,7 @@ class CourseEditLTIFieldsEnabledFlag(ConfigurationModel): course_id = CourseKeyField(max_length=255, db_index=True) @classmethod - @request_cached + @request_cached() def lti_access_to_learners_editable(cls, course_id, is_already_sharing_learner_info): """ Looks at the currently active configuration model to determine whether diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 3361d23c65e..c2645fdbee3 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -18,7 +18,7 @@ from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.django.models import CourseKeyField from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from openedx.core.djangoapps.request_cache.middleware import ns_request_cached +from openedx.core.lib.cache_utils import request_cached Mode = namedtuple('Mode', [ @@ -311,7 +311,7 @@ class CourseMode(models.Model): return [mode.to_tuple() for mode in found_course_modes] @classmethod - @ns_request_cached(CACHE_NAMESPACE) + @request_cached(CACHE_NAMESPACE) def modes_for_course(cls, course_id, include_expired=False, only_selectable=True): """ Returns a list of the non-expired modes for a given course id diff --git a/common/djangoapps/django_comment_common/utils.py b/common/djangoapps/django_comment_common/utils.py index 30384fbee33..5197b5bc9cb 100644 --- a/common/djangoapps/django_comment_common/utils.py +++ b/common/djangoapps/django_comment_common/utils.py @@ -11,7 +11,7 @@ from django_comment_common.models import ( Role ) from openedx.core.djangoapps.course_groups.cohorts import get_legacy_discussion_settings -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached from .models import CourseDiscussionSettings @@ -110,7 +110,7 @@ def are_permissions_roles_seeded(course_id): return True -@request_cached +@request_cached() def get_course_discussion_settings(course_key): try: course_discussion_settings = CourseDiscussionSettings.objects.get(course_id=course_key) diff --git a/common/djangoapps/edxmako/paths.py b/common/djangoapps/edxmako/paths.py index c8b66e3ff6b..8e57f17c963 100644 --- a/common/djangoapps/edxmako/paths.py +++ b/common/djangoapps/edxmako/paths.py @@ -11,9 +11,9 @@ from django.conf import settings from mako.exceptions import TopLevelLookupException from mako.lookup import TemplateLookup -from openedx.core.djangoapps.request_cache.middleware import request_cached from openedx.core.djangoapps.theming.helpers import get_template as themed_template from openedx.core.djangoapps.theming.helpers import get_template_path_with_theme, strip_site_theme_templates_path +from openedx.core.lib.cache_utils import request_cached from . import LOOKUP @@ -148,7 +148,7 @@ def add_lookup(namespace, directory, package=None, prepend=False): templates.add_directory(directory, prepend=prepend) -@request_cached +@request_cached() def lookup_template(namespace, name): """ Look up a Mako template by namespace and name. diff --git a/common/djangoapps/edxmako/request_context.py b/common/djangoapps/edxmako/request_context.py index 8daeac4698b..5be4c1fd477 100644 --- a/common/djangoapps/edxmako/request_context.py +++ b/common/djangoapps/edxmako/request_context.py @@ -22,8 +22,8 @@ Methods for creating RequestContext for using with Mako templates. from crum import get_current_request from django.template import RequestContext -from openedx.core.djangoapps.request_cache import get_cache -from util.request import safe_get_host +from edx_django_utils.cache import RequestCache +from openedx.core.lib.request_utils import safe_get_host def get_template_request_context(request=None): @@ -38,7 +38,7 @@ def get_template_request_context(request=None): if request is None: return None - request_cache_dict = get_cache('edxmako') + request_cache_dict = RequestCache('edxmako').data cache_key = "request_context" if cache_key in request_cache_dict: return request_cache_dict[cache_key] diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 38f6db2d6c3..4c9b540aaea 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -50,6 +50,7 @@ from six import text_type from slumber.exceptions import HttpClientError, HttpServerError from user_util import user_util +from edx_django_utils.cache import RequestCache import lms.lib.comment_client as cc from student.signals import UNENROLL_DONE, ENROLL_STATUS_CHANGE, ENROLLMENT_TRACK_UPDATED from lms.djangoapps.certificates.models import GeneratedCertificate @@ -62,7 +63,6 @@ from courseware.models import ( from enrollment.api import _default_course_mode from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from openedx.core.djangoapps.request_cache import clear_cache, get_cache from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.xmodule_django.models import NoneToEmptyManager from openedx.core.djangolib.model_mixins import DeletableByUserValue @@ -2069,7 +2069,7 @@ class CourseEnrollment(models.Model): """ # before populating the cache with another bulk set of data, # remove previously cached entries to keep memory usage low. - clear_cache(cls.MODE_CACHE_NAMESPACE) + RequestCache(cls.MODE_CACHE_NAMESPACE).clear() records = cls.objects.filter(user__in=users, course_id=course_key).select_related('user') cache = cls._get_mode_active_request_cache() @@ -2080,9 +2080,9 @@ class CourseEnrollment(models.Model): @classmethod def _get_mode_active_request_cache(cls): """ - Returns the request-specific cache for CourseEnrollment + Returns the request-specific cache for CourseEnrollment as dict. """ - return get_cache(cls.MODE_CACHE_NAMESPACE) + return RequestCache(cls.MODE_CACHE_NAMESPACE).data @classmethod def _get_enrollment_in_request_cache(cls, user, course_key): diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 192426d445b..19f4425cc00 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -10,7 +10,7 @@ from collections import defaultdict from django.contrib.auth.models import User from opaque_keys.edx.django.models import CourseKeyField -from openedx.core.djangoapps.request_cache import get_cache +from openedx.core.lib.cache_utils import get_cache from student.models import CourseAccessRole log = logging.getLogger(__name__) diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index 5146c5ce2fc..3db4d2a1fcc 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -24,6 +24,7 @@ from student.models import ( UserProfile, get_retired_email_by_email ) +from openedx.core.lib.request_utils import safe_get_host from student.tests.factories import PendingEmailChangeFactory, RegistrationFactory, UserFactory from student.views import ( SETTING_CHANGE_INITIATED, @@ -33,7 +34,6 @@ from student.views import ( ) from student.views import generate_activation_email_context, send_reactivation_email_for_user from third_party_auth.views import inactive_user_view -from util.request import safe_get_host from util.testing import EventTestMixin diff --git a/common/djangoapps/track/contexts.py b/common/djangoapps/track/contexts.py index 1f615788723..377b9b76f60 100644 --- a/common/djangoapps/track/contexts.py +++ b/common/djangoapps/track/contexts.py @@ -5,7 +5,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from six import text_type -from util.request import COURSE_REGEX +from openedx.core.lib.request_utils import COURSE_REGEX log = logging.getLogger(__name__) diff --git a/common/djangoapps/track/event_transaction_utils.py b/common/djangoapps/track/event_transaction_utils.py index 53b86ddb3bb..6c24f9ede2a 100644 --- a/common/djangoapps/track/event_transaction_utils.py +++ b/common/djangoapps/track/event_transaction_utils.py @@ -4,7 +4,7 @@ used in event tracking. """ from uuid import UUID, uuid4 -from openedx.core.djangoapps.request_cache import get_cache +from openedx.core.lib.cache_utils import get_cache def get_event_transaction_id(): diff --git a/common/djangoapps/util/db.py b/common/djangoapps/util/db.py index 5426e5d84ed..c3386eabe2d 100644 --- a/common/djangoapps/util/db.py +++ b/common/djangoapps/util/db.py @@ -9,7 +9,7 @@ from functools import wraps from django.db import DEFAULT_DB_ALIAS, DatabaseError, Error, transaction -from openedx.core.djangoapps.request_cache import get_cache +from openedx.core.lib.cache_utils import get_cache OUTER_ATOMIC_CACHE_NAME = 'db.outer_atomic' diff --git a/common/djangoapps/util/milestones_helpers.py b/common/djangoapps/util/milestones_helpers.py index ba617b857e8..907f579d6a3 100644 --- a/common/djangoapps/util/milestones_helpers.py +++ b/common/djangoapps/util/milestones_helpers.py @@ -11,8 +11,8 @@ from milestones.services import MilestonesService from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.request_cache import get_cache from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.lib.cache_utils import get_cache from xmodule.modulestore.django import modulestore NAMESPACE_CHOICES = { diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 14a7b55f449..deec0fb5000 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -12,7 +12,8 @@ import logging from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import BlockUsageLocator from six import text_type -from openedx.core.lib.cache_utils import memoize_in_request_cache +from openedx.core.lib.cache_utils import request_cached +from xblock.core import XBlock from xmodule.exceptions import InvalidVersionError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ( @@ -641,7 +642,6 @@ class DraftModuleStore(MongoModuleStore): bulk_record.dirty = True self.collection.remove({'_id': {'$in': to_be_deleted}}, safe=self.collection.safe) - @memoize_in_request_cache('request_cache') def has_changes(self, xblock): """ Check if the subtree rooted at xblock has any drafts and thus may possibly have changes @@ -649,6 +649,18 @@ class DraftModuleStore(MongoModuleStore): :return: True if there are any drafts anywhere in the subtree under xblock (a weaker condition than for other stores) """ + return self._cached_has_changes(self.request_cache, xblock) + + @request_cached( + # use the XBlock's location value in the cache key + arg_map_function=lambda arg: unicode(arg.location if isinstance(arg, XBlock) else arg), + # use this store's request_cache + request_cache_getter=lambda args, kwargs: args[1], + ) + def _cached_has_changes(self, request_cache, xblock): + """ + Internal has_changes method that caches the result. + """ # don't check children if this block has changes (is not public) if getattr(xblock, 'is_draft', False): return True diff --git a/common/lib/xmodule/xmodule/partitions/partitions_service.py b/common/lib/xmodule/xmodule/partitions/partitions_service.py index cd48aa4dc08..51a2b79cec4 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions_service.py +++ b/common/lib/xmodule/xmodule/partitions/partitions_service.py @@ -7,7 +7,7 @@ from django.conf import settings from django.utils.translation import ugettext_lazy as _ import logging -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached from xmodule.partitions.partitions import UserPartition, UserPartitionError, ENROLLMENT_TRACK_PARTITION_ID from xmodule.modulestore.django import modulestore @@ -17,7 +17,7 @@ log = logging.getLogger(__name__) FEATURES = getattr(settings, 'FEATURES', {}) -@request_cached +@request_cached() def get_all_partitions_for_course(course, active_only=False): """ A method that returns all `UserPartitions` associated with a course, as a List. diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index adc3ba29a72..1bc57e0d7fb 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -25,7 +25,7 @@ from lxml import etree from opaque_keys.edx.locator import AssetLocator from openedx.core.djangoapps.video_config.models import HLSPlaybackEnabledFlag from openedx.core.djangoapps.video_pipeline.config.waffle import waffle_flags, DEPRECATE_YOUTUBE -from openedx.core.lib.cache_utils import memoize_in_request_cache +from openedx.core.lib.cache_utils import request_cached from openedx.core.lib.license import LicenseMixin from xblock.completable import XBlockCompletionMode from xblock.core import XBlock @@ -1053,8 +1053,11 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler """ return self.runtime.service(self, "request_cache") - @memoize_in_request_cache('request_cache') - def get_cached_val_data_for_course(self, video_profile_names, course_id): + @classmethod + @request_cached( + request_cache_getter=lambda args, kwargs: args[1], + ) + def get_cached_val_data_for_course(cls, request_cache, video_profile_names, course_id): """ Returns the VAL data for the requested video profiles for the given course. """ @@ -1087,7 +1090,11 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler video_profile_names.append('hls') # get and cache bulk VAL data for course - val_course_data = self.get_cached_val_data_for_course(video_profile_names, self.location.course_key) + val_course_data = self.get_cached_val_data_for_course( + self.request_cache, + video_profile_names, + self.location.course_key, + ) val_video_data = val_course_data.get(self.edx_video_id, {}) # Get the encoded videos if data from VAL is found diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index 972471885f9..503a812fb21 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -9,7 +9,7 @@ from ccx_keys.locator import CCXBlockUsageLocator, CCXLocator from django.db import transaction from opaque_keys.edx.keys import CourseKey, UsageKey -from openedx.core.djangoapps.request_cache import get_cache +from openedx.core.lib.cache_utils import get_cache from courseware.field_overrides import FieldOverrideProvider from lms.djangoapps.ccx.models import CcxFieldOverride, CustomCourseForEdX diff --git a/lms/djangoapps/course_wiki/middleware.py b/lms/djangoapps/course_wiki/middleware.py index f0efe51dd3e..fe742082864 100644 --- a/lms/djangoapps/course_wiki/middleware.py +++ b/lms/djangoapps/course_wiki/middleware.py @@ -10,9 +10,9 @@ from wiki.models import reverse from courseware.access import has_access from courseware.courses import get_course_overview_with_access, get_course_with_access +from openedx.core.lib.request_utils import course_id_from_url from openedx.features.enterprise_support.api import get_enterprise_consent_url from student.models import CourseEnrollment -from util.request import course_id_from_url class WikiAccessMiddleware(object): diff --git a/lms/djangoapps/courseware/context_processor.py b/lms/djangoapps/courseware/context_processor.py index c54df54dc55..5b9c7c460cc 100644 --- a/lms/djangoapps/courseware/context_processor.py +++ b/lms/djangoapps/courseware/context_processor.py @@ -5,9 +5,9 @@ This is meant to simplify the process of sending user preferences (espec. time_z to the templates without having to append every view file. """ -from openedx.core.djangoapps.request_cache import get_cache from openedx.core.djangoapps.user_api.errors import UserAPIInternalError, UserNotFound from openedx.core.djangoapps.user_api.preferences.api import get_user_preferences +from openedx.core.lib.cache_utils import get_cache RETRIEVABLE_PREFERENCES = { 'user_timezone': 'time_zone', diff --git a/lms/djangoapps/courseware/middleware.py b/lms/djangoapps/courseware/middleware.py index 4ca9a804beb..81d053327e6 100644 --- a/lms/djangoapps/courseware/middleware.py +++ b/lms/djangoapps/courseware/middleware.py @@ -5,7 +5,7 @@ Middleware for the courseware app from django.shortcuts import redirect from lms.djangoapps.courseware.exceptions import Redirect -from util.request import COURSE_REGEX +from openedx.core.lib.request_utils import COURSE_REGEX class RedirectMiddleware(object): diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index cd602d65c07..346adb26bf4 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -33,6 +33,7 @@ from lxml import etree from mock import MagicMock, Mock, patch from path import Path as path +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase from openedx.core.lib.tests import attr from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel from openedx.core.djangoapps.video_pipeline.config.waffle import waffle_flags, DEPRECATE_YOUTUBE @@ -1327,7 +1328,7 @@ class TestEditorSavedMethod(BaseTestXmodule): @ddt.ddt -class TestVideoDescriptorStudentViewJson(TestCase): +class TestVideoDescriptorStudentViewJson(CacheIsolationTestCase): """ Tests for the student_view_data method on VideoDescriptor. """ diff --git a/lms/djangoapps/discussion/tests/test_signals.py b/lms/djangoapps/discussion/tests/test_signals.py index d99aee9d5c6..e22fc38d5dd 100644 --- a/lms/djangoapps/discussion/tests/test_signals.py +++ b/lms/djangoapps/discussion/tests/test_signals.py @@ -2,8 +2,8 @@ from django.test import TestCase import mock from django_comment_common import signals, models +from edx_django_utils.cache import RequestCache from lms.djangoapps.discussion.signals.handlers import ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY -import openedx.core.djangoapps.request_cache as request_cache from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory, SiteConfigurationFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -76,7 +76,7 @@ class CoursePublishHandlerTestCase(ModuleStoreTestCase): self._assert_discussion_id_map(course_key, {}) # create discussion block - request_cache.clear_cache(name=None) + RequestCache().clear() discussion_id = 'discussion1' discussion_block = ItemFactory.create( parent_location=course.location, diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index 8dba693a2a5..28c5cb4d59a 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/lms/djangoapps/django_comment_client/permissions.py @@ -12,7 +12,7 @@ from django_comment_common.models import CourseDiscussionSettings, all_permissio from django_comment_common.utils import get_course_discussion_settings from lms.djangoapps.teams.models import CourseTeam from lms.lib.comment_client import Thread -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached def has_permission(user, permission, course_id=None): @@ -33,7 +33,7 @@ def has_permission(user, permission, course_id=None): CONDITIONS = ['is_open', 'is_author', 'is_question_author', 'is_team_member_if_applicable'] -@request_cached +@request_cached() def get_team(commentable_id): """ Returns the team that the commentable_id belongs to if it exists. Returns None otherwise. """ try: diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 53c0a7da2e3..064ecd89246 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -27,7 +27,7 @@ from django_comment_common.models import ( ) from django_comment_common.utils import get_course_discussion_settings from openedx.core.djangoapps.course_groups.cohorts import get_cohort_id, get_cohort_names, is_course_cohorted -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached from student.models import get_user_by_username_or_email from student.roles import GlobalStaff from xmodule.modulestore.django import modulestore @@ -137,7 +137,7 @@ def get_accessible_discussion_xblocks(course, user, include_all=False): return get_accessible_discussion_xblocks_by_course_id(course.id, user, include_all=include_all) -@request_cached +@request_cached() def get_accessible_discussion_xblocks_by_course_id(course_id, user=None, include_all=False): # pylint: disable=invalid-name """ Return a list of all valid discussion xblocks in this course. @@ -169,7 +169,7 @@ class DiscussionIdMapIsNotCached(Exception): pass -@request_cached +@request_cached() def get_cached_discussion_key(course_id, discussion_id): """ Returns the usage key of the discussion xblock associated with discussion_id if it is cached. If the discussion id @@ -236,7 +236,7 @@ def get_discussion_id_map_by_course_id(course_id, user): return dict(map(get_discussion_id_map_entry, xblocks)) -@request_cached +@request_cached() def _get_item_from_modulestore(key): return modulestore().get_item(key) @@ -856,7 +856,7 @@ def get_group_id_for_comments_service(request, course_key, commentable_id=None): return None -@request_cached +@request_cached() def get_group_id_for_user_from_cache(user, course_id): """ Caches the results of get_group_id_for_user, but serializes the course_id diff --git a/lms/djangoapps/grades/config/models.py b/lms/djangoapps/grades/config/models.py index 485d9bc0e7a..df0ec8c31ce 100644 --- a/lms/djangoapps/grades/config/models.py +++ b/lms/djangoapps/grades/config/models.py @@ -8,7 +8,7 @@ from django.db.models import BooleanField, IntegerField, TextField from opaque_keys.edx.django.models import CourseKeyField from six import text_type -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached class PersistentGradesEnabledFlag(ConfigurationModel): @@ -22,7 +22,7 @@ class PersistentGradesEnabledFlag(ConfigurationModel): enabled_for_all_courses = BooleanField(default=False) @classmethod - @request_cached + @request_cached() def feature_enabled(cls, course_id=None): """ Looks at the currently active configuration model to determine whether diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index cdb557fa5cb..6cfcf8b7542 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -22,7 +22,7 @@ from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField from opaque_keys.edx.keys import CourseKey, UsageKey from coursewarehistoryextended.fields import UnsignedBigIntAutoField, UnsignedBigIntOneToOneField -from openedx.core.djangoapps.request_cache import get_cache +from openedx.core.lib.cache_utils import get_cache import events diff --git a/lms/djangoapps/grades/scores.py b/lms/djangoapps/grades/scores.py index a224e31b542..f095cf83835 100644 --- a/lms/djangoapps/grades/scores.py +++ b/lms/djangoapps/grades/scores.py @@ -5,7 +5,7 @@ from logging import getLogger from xblock.core import XBlock -from openedx.core.lib.cache_utils import memoized +from openedx.core.lib.cache_utils import process_cached from xmodule.graders import ProblemScore from numpy import around @@ -262,7 +262,7 @@ def _get_explicit_graded(block): return True if field_value is None else field_value -@memoized +@process_cached def _block_types_possibly_scored(): """ Returns the block types that could have a score. diff --git a/lms/djangoapps/mobile_api/middleware.py b/lms/djangoapps/mobile_api/middleware.py index 79a91b7f3a6..15b403f432e 100644 --- a/lms/djangoapps/mobile_api/middleware.py +++ b/lms/djangoapps/mobile_api/middleware.py @@ -8,10 +8,10 @@ from django.core.cache import cache from django.http import HttpResponse from pytz import UTC -from openedx.core.djangoapps.request_cache import get_cache from mobile_api.mobile_platform import MobilePlatform from mobile_api.models import AppVersionConfig from mobile_api.utils import parsed_version +from openedx.core.lib.cache_utils import get_cache from openedx.core.lib.mobile_utils import is_request_from_mobile_app diff --git a/lms/djangoapps/teams/search_indexes.py b/lms/djangoapps/teams/search_indexes.py index 085b3d51bd9..187f5a9d5ce 100644 --- a/lms/djangoapps/teams/search_indexes.py +++ b/lms/djangoapps/teams/search_indexes.py @@ -11,7 +11,7 @@ from elasticsearch.exceptions import ConnectionError from search.search_engine_base import SearchEngine from lms.djangoapps.teams.models import CourseTeam -from openedx.core.djangoapps.request_cache import get_request_or_stub +from openedx.core.lib.request_utils import get_request_or_stub from .errors import ElasticSearchConnectionError from .serializers import CourseTeamSerializer diff --git a/openedx/core/djangoapps/content/block_structure/config/__init__.py b/openedx/core/djangoapps/content/block_structure/config/__init__.py index 637df079144..063fa4de536 100644 --- a/openedx/core/djangoapps/content/block_structure/config/__init__.py +++ b/openedx/core/djangoapps/content/block_structure/config/__init__.py @@ -3,7 +3,7 @@ This module contains various configuration settings via waffle switches for the Block Structure framework. """ from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached from .models import BlockStructureConfiguration @@ -24,7 +24,7 @@ def waffle(): return WaffleSwitchNamespace(name=WAFFLE_NAMESPACE, log_prefix=u'BlockStructure: ') -@request_cached +@request_cached() def num_versions_to_keep(): """ Returns and caches the current setting for num_versions_to_keep. @@ -32,7 +32,7 @@ def num_versions_to_keep(): return BlockStructureConfiguration.current().num_versions_to_keep -@request_cached +@request_cached() def cache_timeout_in_seconds(): """ Returns and caches the current setting for cache_timeout_in_seconds. diff --git a/openedx/core/djangoapps/content/block_structure/transformer_registry.py b/openedx/core/djangoapps/content/block_structure/transformer_registry.py index 2a57d64c9bb..d90fe57b3f0 100644 --- a/openedx/core/djangoapps/content/block_structure/transformer_registry.py +++ b/openedx/core/djangoapps/content/block_structure/transformer_registry.py @@ -6,7 +6,7 @@ from base64 import b64encode from hashlib import sha1 from openedx.core.lib.plugins import PluginManager -from openedx.core.lib.cache_utils import memoized +from openedx.core.lib.cache_utils import process_cached class TransformerRegistry(PluginManager): @@ -35,7 +35,7 @@ class TransformerRegistry(PluginManager): return set() @classmethod - @memoized + @process_cached def get_write_version_hash(cls): """ Returns a deterministic hash value of the WRITE_VERSION of all diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index f287d18458f..599f8161a4e 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -16,8 +16,8 @@ from django.dispatch import receiver from django.http import Http404 from django.utils.translation import ugettext as _ from eventtracking import tracker -from openedx.core.djangoapps.request_cache import clear_cache, get_cache -from openedx.core.djangoapps.request_cache.middleware import request_cached +from edx_django_utils.cache import RequestCache +from openedx.core.lib.cache_utils import request_cached from student.models import get_user_by_username_or_email from .models import ( @@ -175,8 +175,8 @@ def bulk_cache_cohorts(course_key, users): """ # before populating the cache with another bulk set of data, # remove previously cached entries to keep memory usage low. - clear_cache(COHORT_CACHE_NAMESPACE) - cache = get_cache(COHORT_CACHE_NAMESPACE) + RequestCache(COHORT_CACHE_NAMESPACE).clear() + cache = RequestCache(COHORT_CACHE_NAMESPACE).data if is_course_cohorted(course_key): cohorts_by_user = { @@ -215,7 +215,7 @@ def get_cohort(user, course_key, assign=True, use_cached=False): Raises: ValueError if the CourseKey doesn't exist. """ - cache = get_cache(COHORT_CACHE_NAMESPACE) + cache = RequestCache(COHORT_CACHE_NAMESPACE).data cache_key = _cohort_cache_key(user.id, course_key) if use_cached and cache_key in cache: @@ -514,7 +514,7 @@ def get_group_info_for_cohort(cohort, use_cached=False): use_cached=True to use the cached value instead of fetching from the database. """ - cache = get_cache(u"cohorts.get_group_info_for_cohort") + cache = RequestCache(u"cohorts.get_group_info_for_cohort").data cache_key = unicode(cohort.id) if use_cached and cache_key in cache: @@ -565,7 +565,7 @@ def is_last_random_cohort(user_group): return len(random_cohorts) == 1 and random_cohorts[0].name == user_group.name -@request_cached +@request_cached() def _get_course_cohort_settings(course_key): """ Return cohort settings for a course. NOTE that the only non-deprecated fields in diff --git a/openedx/core/djangoapps/credit/models.py b/openedx/core/djangoapps/credit/models.py index bbc7f7e4cde..fff0159774f 100644 --- a/openedx/core/djangoapps/credit/models.py +++ b/openedx/core/djangoapps/credit/models.py @@ -24,7 +24,7 @@ from jsonfield.fields import JSONField from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import CourseKeyField -from openedx.core.djangoapps.request_cache.middleware import ns_request_cached +from openedx.core.lib.cache_utils import request_cached CREDIT_PROVIDER_ID_REGEX = r"[a-z,A-Z,0-9,\-]+" log = logging.getLogger(__name__) @@ -335,7 +335,7 @@ class CreditRequirement(TimeStampedModel): return credit_requirement, created @classmethod - @ns_request_cached(CACHE_NAMESPACE) + @request_cached(namespace=CACHE_NAMESPACE) def get_course_requirements(cls, course_key, namespace=None, name=None): """ Get credit requirements of a given course. diff --git a/openedx/core/djangoapps/embargo/middleware.py b/openedx/core/djangoapps/embargo/middleware.py index 1b0f7f194c2..5d4dbceee53 100644 --- a/openedx/core/djangoapps/embargo/middleware.py +++ b/openedx/core/djangoapps/embargo/middleware.py @@ -34,7 +34,7 @@ from django.urls import reverse from django.shortcuts import redirect from ipware.ip import get_ip -from util.request import course_id_from_url +from openedx.core.lib.request_utils import course_id_from_url from . import api as embargo_api from .models import IPFilter diff --git a/openedx/core/djangoapps/oauth_dispatch/models.py b/openedx/core/djangoapps/oauth_dispatch/models.py index 9ffff624627..0c4285e3daa 100644 --- a/openedx/core/djangoapps/oauth_dispatch/models.py +++ b/openedx/core/djangoapps/oauth_dispatch/models.py @@ -12,7 +12,7 @@ from organizations.models import Organization from pytz import utc from openedx.core.djangoapps.oauth_dispatch.toggles import ENFORCE_JWT_SCOPES -from openedx.core.djangoapps.request_cache import get_request_or_stub +from openedx.core.lib.request_utils import get_request_or_stub class RestrictedApplication(models.Model): diff --git a/openedx/core/djangoapps/request_cache/__init__.py b/openedx/core/djangoapps/request_cache/__init__.py deleted file mode 100644 index 1e50ed5f3dc..00000000000 --- a/openedx/core/djangoapps/request_cache/__init__.py +++ /dev/null @@ -1,83 +0,0 @@ -""" -A cache that is cleared after every request. - -This module requires that :class:`request_cache.middleware.RequestCache` -is installed in order to clear the cache after each request. -""" - -import logging -from urlparse import urlparse - -from celery.signals import task_postrun -import crum -from django.conf import settings -from django.test.client import RequestFactory -from edx_django_utils.cache import RequestCache - -from openedx.core.djangoapps.request_cache import middleware - - -log = logging.getLogger(__name__) - - -@task_postrun.connect -def clear_request_cache(**kwargs): # pylint: disable=unused-argument - """ - Once a celery task completes, clear the request cache to - prevent memory leaks. - """ - if getattr(settings, 'CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION', True): - RequestCache.clear_all_namespaces() - - -def get_cache(name): - """ - Return the request cache named ``name``. - - Arguments: - name (str): The name of the request cache to load - - Returns: dict - """ - assert name is not None - return RequestCache(name).data - - -def clear_cache(name): - """ - Clears the request cache named ``name``. - - Arguments: - name (str): The name of the request cache to clear - """ - RequestCache(name).clear() - - -def get_request_or_stub(): - """ - Return the current request or a stub request. - - If called outside the context of a request, construct a fake - request that can be used to build an absolute URI. - - This is useful in cases where we need to pass in a request object - but don't have an active request (for example, in tests, celery tasks, and XBlocks). - """ - request = crum.get_current_request() - - if request is None: - - # The settings SITE_NAME may contain a port number, so we need to - # parse the full URL. - full_url = "http://{site_name}".format(site_name=settings.SITE_NAME) - parsed_url = urlparse(full_url) - - # Construct the fake request. This can be used to construct absolute - # URIs to other paths. - return RequestFactory( - SERVER_NAME=parsed_url.hostname, - SERVER_PORT=parsed_url.port or 80, - ).get("/") - - else: - return request diff --git a/openedx/core/djangoapps/request_cache/middleware.py b/openedx/core/djangoapps/request_cache/middleware.py deleted file mode 100644 index 90d5209098e..00000000000 --- a/openedx/core/djangoapps/request_cache/middleware.py +++ /dev/null @@ -1,82 +0,0 @@ -""" -The middleware for the edx-platform version of the RequestCache has been -removed in favor of the RequestCache found in edx-django-utils. - -TODO: This file still contains request cache related decorators that -should be moved out of this middleware file. -""" -from django.utils.encoding import force_text -from edx_django_utils.cache import RequestCache - - -def request_cached(f): - """ - A decorator for wrapping a function and automatically handles caching its return value, as well as returning - that cached value for subsequent calls to the same function, with the same parameters, within a given request. - - Notes: - - we convert arguments and keyword arguments to their string form to build the cache key, so if you have - args/kwargs that can't be converted to strings, you're gonna have a bad time (don't do it) - - cache key cardinality depends on the args/kwargs, so if you're caching a function that takes five arguments, - you might have deceptively low cache efficiency. prefer function with fewer arguments. - - we use the default request cache, not a named request cache (this shouldn't matter, but just mentioning it) - - benchmark, benchmark, benchmark! if you never measure, how will you know you've improved? or regressed? - - Arguments: - f (func): the function to wrap - - Returns: - func: a wrapper function which will call the wrapped function, passing in the same args/kwargs, - cache the value it returns, and return that cached value for subsequent calls with the - same args/kwargs within a single request - """ - return ns_request_cached()(f) - - -def ns_request_cached(namespace=None): - """ - Same as request_cached above, except an optional namespace can be passed in to compartmentalize the cache. - - Arguments: - namespace (string): An optional namespace to use for the cache. Useful if the caller wants to manage - their own sub-cache by, for example, calling RequestCache(namespace=NAMESPACE).clear() for their own - namespace. - """ - def outer_wrapper(f): - """ - Outer wrapper that decorates the given function - - Arguments: - f (func): the function to wrap - """ - def inner_wrapper(*args, **kwargs): - """ - Wrapper function to decorate with. - """ - # Check to see if we have a result in cache. If not, invoke our wrapped - # function. Cache and return the result to the caller. - request_cache = RequestCache(namespace) - cache_key = _func_call_cache_key(f, *args, **kwargs) - - cached_response = request_cache.get_cached_response(cache_key) - if cached_response.is_found: - return cached_response.value - - result = f(*args, **kwargs) - request_cache.set(cache_key, result) - return result - - return inner_wrapper - return outer_wrapper - - -def _func_call_cache_key(func, *args, **kwargs): - """ - Returns a cache key based on the function's module - the function's name, and a stringified list of arguments - and a query string-style stringified list of keyword arguments. - """ - converted_args = map(force_text, args) - converted_kwargs = map(force_text, reduce(list.__add__, map(list, sorted(kwargs.iteritems())), [])) - cache_keys = [func.__module__, func.func_name] + converted_args + converted_kwargs - return u'.'.join(cache_keys) diff --git a/openedx/core/djangoapps/request_cache/tests.py b/openedx/core/djangoapps/request_cache/tests.py deleted file mode 100644 index 68fc74faa66..00000000000 --- a/openedx/core/djangoapps/request_cache/tests.py +++ /dev/null @@ -1,250 +0,0 @@ -# -*- coding: utf-8 -*- -""" -Tests for the request cache. -""" -from celery.task import task -from django.conf import settings -from django.test import TestCase -from django.test.utils import override_settings -from edx_django_utils.cache import RequestCache -from mock import Mock - -from openedx.core.djangoapps.request_cache import get_request_or_stub -from openedx.core.djangoapps.request_cache.middleware import request_cached -from xmodule.modulestore.django import modulestore - - -class TestRequestCache(TestCase): - """ - Tests for request cache helpers and decorators. - """ - - def test_get_request_or_stub(self): - """ - Outside the context of the request, we should still get a request - that allows us to build an absolute URI. - """ - stub = get_request_or_stub() - expected_url = "http://{site_name}/foobar".format(site_name=settings.SITE_NAME) - self.assertEqual(stub.build_absolute_uri("foobar"), expected_url) - - @task - def _dummy_task(self): - """ Create a task that adds stuff to the request cache. """ - cache = {"course_cache": "blah blah blah"} - modulestore().request_cache.data.update(cache) - - @override_settings(CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION=True) - def test_clear_cache_celery(self): - """ Test that the request cache is cleared after a task is run. """ - self._dummy_task.apply(args=(self,)).get() - self.assertEqual(modulestore().request_cache.data, {}) - - def test_request_cached_miss_and_then_hit(self): - """ - Ensure that after a cache miss, we fill the cache and can hit it. - """ - RequestCache.clear_all_namespaces() - - to_be_wrapped = Mock() - to_be_wrapped.return_value = 42 - self.assertEqual(to_be_wrapped.call_count, 0) - - def mock_wrapper(*args, **kwargs): - """Simple wrapper to let us decorate our mock.""" - return to_be_wrapped(*args, **kwargs) - - wrapped = request_cached(mock_wrapper) - result = wrapped() - self.assertEqual(result, 42) - self.assertEqual(to_be_wrapped.call_count, 1) - - result = wrapped() - self.assertEqual(result, 42) - self.assertEqual(to_be_wrapped.call_count, 1) - - def test_request_cached_with_caches_despite_changing_wrapped_result(self): - """ - Ensure that after caching a result, we always send it back, even if the underlying result changes. - """ - RequestCache.clear_all_namespaces() - - to_be_wrapped = Mock() - to_be_wrapped.side_effect = [1, 2, 3] - self.assertEqual(to_be_wrapped.call_count, 0) - - def mock_wrapper(*args, **kwargs): - """Simple wrapper to let us decorate our mock.""" - return to_be_wrapped(*args, **kwargs) - - wrapped = request_cached(mock_wrapper) - result = wrapped() - self.assertEqual(result, 1) - self.assertEqual(to_be_wrapped.call_count, 1) - - result = wrapped() - self.assertEqual(result, 1) - self.assertEqual(to_be_wrapped.call_count, 1) - - direct_result = mock_wrapper() - self.assertEqual(direct_result, 2) - self.assertEqual(to_be_wrapped.call_count, 2) - - result = wrapped() - self.assertEqual(result, 1) - self.assertEqual(to_be_wrapped.call_count, 2) - - direct_result = mock_wrapper() - self.assertEqual(direct_result, 3) - self.assertEqual(to_be_wrapped.call_count, 3) - - def test_request_cached_with_changing_args(self): - """ - Ensure that calling a decorated function with different positional arguments - will not use a cached value invoked by a previous call with different arguments. - """ - RequestCache.clear_all_namespaces() - - to_be_wrapped = Mock() - to_be_wrapped.side_effect = [1, 2, 3, 4, 5, 6] - self.assertEqual(to_be_wrapped.call_count, 0) - - def mock_wrapper(*args, **kwargs): - """Simple wrapper to let us decorate our mock.""" - return to_be_wrapped(*args, **kwargs) - - wrapped = request_cached(mock_wrapper) - - # This will be a miss, and make an underlying call. - result = wrapped(1) - self.assertEqual(result, 1) - self.assertEqual(to_be_wrapped.call_count, 1) - - # This will be a miss, and make an underlying call. - result = wrapped(2) - self.assertEqual(result, 2) - self.assertEqual(to_be_wrapped.call_count, 2) - - # This is bypass of the decorator. - direct_result = mock_wrapper(3) - self.assertEqual(direct_result, 3) - self.assertEqual(to_be_wrapped.call_count, 3) - - # These will be hits, and not make an underlying call. - result = wrapped(1) - self.assertEqual(result, 1) - self.assertEqual(to_be_wrapped.call_count, 3) - - result = wrapped(2) - self.assertEqual(result, 2) - self.assertEqual(to_be_wrapped.call_count, 3) - - def test_request_cached_with_changing_kwargs(self): - """ - Ensure that calling a decorated function with different keyword arguments - will not use a cached value invoked by a previous call with different arguments. - """ - RequestCache.clear_all_namespaces() - - to_be_wrapped = Mock() - to_be_wrapped.side_effect = [1, 2, 3, 4, 5, 6] - self.assertEqual(to_be_wrapped.call_count, 0) - - def mock_wrapper(*args, **kwargs): - """Simple wrapper to let us decorate our mock.""" - return to_be_wrapped(*args, **kwargs) - - wrapped = request_cached(mock_wrapper) - - # This will be a miss, and make an underlying call. - result = wrapped(1, foo=1) - self.assertEqual(result, 1) - self.assertEqual(to_be_wrapped.call_count, 1) - - # This will be a miss, and make an underlying call. - result = wrapped(2, foo=2) - self.assertEqual(result, 2) - self.assertEqual(to_be_wrapped.call_count, 2) - - # This is bypass of the decorator. - direct_result = mock_wrapper(3, foo=3) - self.assertEqual(direct_result, 3) - self.assertEqual(to_be_wrapped.call_count, 3) - - # These will be hits, and not make an underlying call. - result = wrapped(1, foo=1) - self.assertEqual(result, 1) - self.assertEqual(to_be_wrapped.call_count, 3) - - result = wrapped(2, foo=2) - self.assertEqual(result, 2) - self.assertEqual(to_be_wrapped.call_count, 3) - - # Since we're changing foo, this will be a miss. - result = wrapped(2, foo=5) - self.assertEqual(result, 4) - self.assertEqual(to_be_wrapped.call_count, 4) - - def test_request_cached_mixed_unicode_str_args(self): - """ - Ensure that request_cached can work with mixed str and Unicode parameters. - """ - RequestCache.clear_all_namespaces() - - def dummy_function(arg1, arg2): - """ - A dummy function that expects an str and unicode arguments. - """ - assert isinstance(arg1, str), 'First parameter has to be of type `str`' - assert isinstance(arg2, unicode), 'Second parameter has to be of type `unicode`' - return True - - self.assertTrue(dummy_function('Hello', u'World'), 'Should be callable with ASCII chars') - self.assertTrue(dummy_function('H∂llå', u'Wørld'), 'Should be callable with non-ASCII chars') - - wrapped = request_cached(dummy_function) - - self.assertTrue(wrapped('Hello', u'World'), 'Wrapper should handle ASCII only chars') - self.assertTrue(wrapped('H∂llå', u'Wørld'), 'Wrapper should handle non-ASCII chars') - - def test_request_cached_with_none_result(self): - """ - Ensure that calling a decorated function that returns None - properly caches the result and doesn't recall the underlying - function. - """ - RequestCache.clear_all_namespaces() - - to_be_wrapped = Mock() - to_be_wrapped.side_effect = [None, None, None, 1, 1] - self.assertEqual(to_be_wrapped.call_count, 0) - - def mock_wrapper(*args, **kwargs): - """Simple wrapper to let us decorate our mock.""" - return to_be_wrapped(*args, **kwargs) - - wrapped = request_cached(mock_wrapper) - - # This will be a miss, and make an underlying call. - result = wrapped(1) - self.assertEqual(result, None) - self.assertEqual(to_be_wrapped.call_count, 1) - - # This will be a miss, and make an underlying call. - result = wrapped(2) - self.assertEqual(result, None) - self.assertEqual(to_be_wrapped.call_count, 2) - - # This is bypass of the decorator. - direct_result = mock_wrapper(3) - self.assertEqual(direct_result, None) - self.assertEqual(to_be_wrapped.call_count, 3) - - # These will be hits, and not make an underlying call. - result = wrapped(1) - self.assertEqual(result, None) - self.assertEqual(to_be_wrapped.call_count, 3) - - result = wrapped(2) - self.assertEqual(result, None) - self.assertEqual(to_be_wrapped.call_count, 3) diff --git a/openedx/core/djangoapps/schedules/content_highlights.py b/openedx/core/djangoapps/schedules/content_highlights.py index 777c5a581ad..be0e40c1a86 100644 --- a/openedx/core/djangoapps/schedules/content_highlights.py +++ b/openedx/core/djangoapps/schedules/content_highlights.py @@ -8,7 +8,7 @@ from courseware.module_render import get_module_for_descriptor from courseware.model_data import FieldDataCache from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG from openedx.core.djangoapps.schedules.exceptions import CourseUpdateDoesNotExist -from openedx.core.djangoapps.request_cache import get_request_or_stub +from openedx.core.lib.request_utils import get_request_or_stub from xmodule.modulestore.django import modulestore diff --git a/openedx/core/djangoapps/theming/helpers.py b/openedx/core/djangoapps/theming/helpers.py index 9fcfa5bcf32..a7d298f0aae 100644 --- a/openedx/core/djangoapps/theming/helpers.py +++ b/openedx/core/djangoapps/theming/helpers.py @@ -20,12 +20,12 @@ from openedx.core.djangoapps.theming.helpers_dirs import ( get_theme_dirs, get_themes_unchecked ) -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached logger = getLogger(__name__) # pylint: disable=invalid-name -@request_cached +@request_cached() def get_template_path(relative_path, **kwargs): """ This is a proxy function to hide microsite_configuration behind comprehensive theming. diff --git a/openedx/core/djangoapps/user_api/course_tag/api.py b/openedx/core/djangoapps/user_api/course_tag/api.py index cb8f2a997c2..ab58f439556 100644 --- a/openedx/core/djangoapps/user_api/course_tag/api.py +++ b/openedx/core/djangoapps/user_api/course_tag/api.py @@ -8,7 +8,7 @@ UserCourseTag model. """ from collections import defaultdict -from openedx.core.djangoapps.request_cache import get_cache +from openedx.core.lib.cache_utils import get_cache from ..models import UserCourseTag # Scopes diff --git a/openedx/core/djangoapps/util/signals.py b/openedx/core/djangoapps/util/signals.py index 7006db11e71..95f710bb66e 100644 --- a/openedx/core/djangoapps/util/signals.py +++ b/openedx/core/djangoapps/util/signals.py @@ -1,14 +1,18 @@ """ Signal handler for exceptions. """ +# pylint: disable=unused-argument import logging +from celery.signals import task_postrun +from django.conf import settings from django.core.signals import got_request_exception from django.dispatch import receiver +from edx_django_utils.cache import RequestCache @receiver(got_request_exception) -def record_request_exception(sender, **kwargs): # pylint: disable=unused-argument +def record_request_exception(sender, **kwargs): """ Logs the stack trace whenever an exception occurs in processing a request. @@ -16,3 +20,13 @@ def record_request_exception(sender, **kwargs): # pylint: disable=unused-argume logging.exception("Uncaught exception from {sender}".format( sender=sender )) + + +@task_postrun.connect +def _clear_request_cache(**kwargs): + """ + Once a celery task completes, clear the request cache to + prevent memory leaks. + """ + if getattr(settings, 'CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION', True): + RequestCache.clear_all_namespaces() diff --git a/openedx/core/djangoapps/util/tests/test_signals.py b/openedx/core/djangoapps/util/tests/test_signals.py new file mode 100644 index 00000000000..003784aca91 --- /dev/null +++ b/openedx/core/djangoapps/util/tests/test_signals.py @@ -0,0 +1,24 @@ +# pylint: disable=no-member, missing-docstring +from unittest import TestCase +from celery.task import task +from django.test.utils import override_settings + +from edx_django_utils.cache import RequestCache + + +class TestClearRequestCache(TestCase): + """ + Tests _clear_request_cache is called after celery task is run. + """ + def _get_cache(self): + return RequestCache("TestClearRequestCache") + + @task + def _dummy_task(self): + """ A task that adds stuff to the request cache. """ + self._get_cache().set("cache_key", "blah blah") + + @override_settings(CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION=True) + def test_clear_cache_celery(self): + self._dummy_task.apply(args=(self,)).get() + self.assertFalse(self._get_cache().get_cached_response("cache_key").is_found) diff --git a/openedx/core/djangoapps/util/tests/test_user_utils.py b/openedx/core/djangoapps/util/tests/test_user_utils.py index ec76154d281..05fee5b4a2e 100644 --- a/openedx/core/djangoapps/util/tests/test_user_utils.py +++ b/openedx/core/djangoapps/util/tests/test_user_utils.py @@ -1,4 +1,4 @@ -"""Tests for util.request module.""" +"""Tests for util.user_utils module.""" import unittest diff --git a/openedx/core/djangoapps/verified_track_content/models.py b/openedx/core/djangoapps/verified_track_content/models.py index d752ca7c0e7..36e0aeddea6 100644 --- a/openedx/core/djangoapps/verified_track_content/models.py +++ b/openedx/core/djangoapps/verified_track_content/models.py @@ -19,7 +19,7 @@ from openedx.core.djangoapps.course_groups.cohorts import ( is_course_cohorted ) from openedx.core.djangoapps.verified_track_content.tasks import sync_cohort_with_mode -from openedx.core.djangoapps.request_cache.middleware import ns_request_cached +from openedx.core.lib.cache_utils import request_cached from student.models import CourseEnrollment log = logging.getLogger(__name__) @@ -126,7 +126,7 @@ class VerifiedTrackCohortedCourse(models.Model): return None @classmethod - @ns_request_cached(CACHE_NAMESPACE) + @request_cached(namespace=CACHE_NAMESPACE) def is_verified_track_cohort_enabled(cls, course_key): """ Checks whether or not verified track cohort is enabled for the given course. diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index fdf48b3bb6f..f7691264eef 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -69,7 +69,7 @@ import six from opaque_keys.edx.keys import CourseKey from waffle import flag_is_active, switch_is_active -from openedx.core.djangoapps.request_cache import get_cache as get_request_cache +from openedx.core.lib.cache_utils import get_cache as get_request_cache log = logging.getLogger(__name__) diff --git a/openedx/core/djangoapps/waffle_utils/models.py b/openedx/core/djangoapps/waffle_utils/models.py index 82a85b5f8d3..11a2d988a6c 100644 --- a/openedx/core/djangoapps/waffle_utils/models.py +++ b/openedx/core/djangoapps/waffle_utils/models.py @@ -8,7 +8,7 @@ from opaque_keys.edx.django.models import CourseKeyField from six import text_type from config_models.models import ConfigurationModel -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached class WaffleFlagCourseOverrideModel(ConfigurationModel): @@ -26,7 +26,7 @@ class WaffleFlagCourseOverrideModel(ConfigurationModel): override_choice = CharField(choices=OVERRIDE_CHOICES, default=OVERRIDE_CHOICES.on, max_length=3) @classmethod - @request_cached + @request_cached() def override_value(cls, waffle_flag, course_id): """ Returns whether the waffle flag was overridden (on or off) for the diff --git a/openedx/core/lib/cache_utils.py b/openedx/core/lib/cache_utils.py index 983129aff82..ba4324143d1 100644 --- a/openedx/core/lib/cache_utils.py +++ b/openedx/core/lib/cache_utils.py @@ -4,52 +4,119 @@ Utilities related to caching. import collections import cPickle as pickle import functools +import itertools import zlib -from xblock.core import XBlock +from django.utils.encoding import force_text +from edx_django_utils.cache import RequestCache -def memoize_in_request_cache(request_cache_attr_name=None): +def request_cached(namespace=None, arg_map_function=None, request_cache_getter=None): """ - Memoize a method call's results in the request_cache if there's one. Creates the cache key by - joining the unicode of all the args with &; so, if your arg may use the default &, it may - have false hits. + A function decorator that automatically handles caching its return value for + the duration of the request. It returns the cached value for subsequent + calls to the same function, with the same parameters, within a given request. + + Notes: + - We convert arguments and keyword arguments to their string form to build the cache key. So if you have + args/kwargs that can't be converted to strings, you're gonna have a bad time (don't do it). + - Cache key cardinality depends on the args/kwargs. So if you're caching a function that takes five arguments, + you might have deceptively low cache efficiency. Prefer functions with fewer arguments. + - WATCH OUT: Don't use this decorator for instance methods that take in a "self" argument that changes each + time the method is called. This will result in constant cache misses and not provide the performance benefit + you are looking for. Rather, change your instance method to a class method. + - Benchmark, benchmark, benchmark! If you never measure, how will you know you've improved? or regressed? Arguments: - request_cache_attr_name - The name of the field or property in this method's containing - class that stores the request_cache. + namespace (string): An optional namespace to use for the cache. By default, we use the default request cache, + not a namespaced request cache. Since the code automatically creates a unique cache key with the module and + function's name, storing the cached value in the default cache, you won't usually need to specify a + namespace value. + But you can specify a namespace value here if you need to use your own namespaced cache - for example, + if you want to clear out your own cache by calling RequestCache(namespace=NAMESPACE).clear(). + NOTE: This argument is ignored if you supply a ``request_cache_getter``. + arg_map_function (function: arg->string): Function to use for mapping the wrapped function's arguments to + strings to use in the cache key. If not provided, defaults to force_text, which converts the given + argument to a string. + request_cache_getter (function: args, kwargs->RequestCache): Function that returns the RequestCache to use. + If not provided, defaults to edx_django_utils.cache.RequestCache. If ``request_cache_getter`` returns None, + the function's return values are not cached. + + Returns: + func: a wrapper function which will call the wrapped function, passing in the same args/kwargs, + cache the value it returns, and return that cached value for subsequent calls with the + same args/kwargs within a single request. """ - def _decorator(func): - """Outer method decorator.""" - @functools.wraps(func) - def _wrapper(self, *args, **kwargs): + def decorator(f): + """ + Arguments: + f (func): the function to wrap + """ + @functools.wraps(f) + def _decorator(*args, **kwargs): """ - Wraps a method to memoize results. + Arguments: + args, kwargs: values passed into the wrapped function """ - request_cache = getattr(self, request_cache_attr_name, None) + # Check to see if we have a result in cache. If not, invoke our wrapped + # function. Cache and return the result to the caller. + if request_cache_getter: + request_cache = request_cache_getter(args, kwargs) + else: + request_cache = RequestCache(namespace) + if request_cache: - cache_key = '&'.join([hashvalue(arg) for arg in args]) - if cache_key in request_cache.data.setdefault(func.__name__, {}): - return request_cache.data[func.__name__][cache_key] + cache_key = _func_call_cache_key(f, arg_map_function, *args, **kwargs) + cached_response = request_cache.get_cached_response(cache_key) + if cached_response.is_found: + return cached_response.value - result = func(self, *args, **kwargs) + result = f(*args, **kwargs) + + if request_cache: + request_cache.set(cache_key, result) + + return result + + return _decorator + return decorator + + +def _func_call_cache_key(func, arg_map_function, *args, **kwargs): + """ + Returns a cache key based on the function's module, + the function's name, a stringified list of arguments + and a stringified list of keyword arguments. + """ + arg_map_function = arg_map_function or force_text + + converted_args = map(arg_map_function, args) + converted_kwargs = map(arg_map_function, _sorted_kwargs_list(kwargs)) + + cache_keys = [func.__module__, func.func_name] + converted_args + converted_kwargs + return u'.'.join(cache_keys) - request_cache.data[func.__name__][cache_key] = result - return result - else: - return func(self, *args, **kwargs) - return _wrapper - return _decorator +def _sorted_kwargs_list(kwargs): + """ + Returns a unique and deterministic ordered list from the given kwargs. + """ + sorted_kwargs = sorted(kwargs.iteritems()) + sorted_kwargs_list = list(itertools.chain(*sorted_kwargs)) + return sorted_kwargs_list -class memoized(object): # pylint: disable=invalid-name + +class process_cached(object): # pylint: disable=invalid-name """ - Decorator. Caches a function's return value each time it is called. - If called later with the same arguments, the cached value is returned + Decorator to cache the result of a function for the life of a process. + + If the return value of the function for the provided arguments has not + yet been cached, the function will be calculated and cached. If called + later with the same arguments, the cached value is returned (not reevaluated). https://wiki.python.org/moin/PythonDecoratorLibrary#Memoize - WARNING: Only use this memoized decorator for caching data that + WARNING: Only use this process_cached decorator for caching data that is constant throughout the lifetime of a gunicorn worker process, is costly to compute, and is required often. Otherwise, it can lead to unwanted memory leakage. @@ -84,16 +151,6 @@ class memoized(object): # pylint: disable=invalid-name return functools.partial(self.__call__, obj) -def hashvalue(arg): - """ - If arg is an xblock, use its location. otherwise just turn it into a string - """ - if isinstance(arg, XBlock): - return unicode(arg.location) - else: - return unicode(arg) - - def zpickle(data): """Given any data structure, returns a zlib compressed pickled serialization.""" return zlib.compress(pickle.dumps(data, pickle.HIGHEST_PROTOCOL)) @@ -102,3 +159,16 @@ def zpickle(data): def zunpickle(zdata): """Given a zlib compressed pickled serialization, returns the deserialized data.""" return pickle.loads(zlib.decompress(zdata)) + + +def get_cache(name): + """ + Return the request cache named ``name``. + + Arguments: + name (str): The name of the request cache to load + + Returns: dict + """ + assert name is not None + return RequestCache(name).data diff --git a/openedx/core/lib/celery/task_utils.py b/openedx/core/lib/celery/task_utils.py index 3c281aaa0e6..224f03273f4 100644 --- a/openedx/core/lib/celery/task_utils.py +++ b/openedx/core/lib/celery/task_utils.py @@ -1,9 +1,9 @@ from contextlib import contextmanager from crum import CurrentRequestUserMiddleware -from openedx.core.djangoapps.theming.middleware import CurrentSiteThemeMiddleware from django.http import HttpResponse -from openedx.core.djangoapps.request_cache import get_request_or_stub +from openedx.core.djangoapps.theming.middleware import CurrentSiteThemeMiddleware +from openedx.core.lib.request_utils import get_request_or_stub @contextmanager diff --git a/openedx/core/lib/plugins.py b/openedx/core/lib/plugins.py index 6447cb4f960..5506d4f596f 100644 --- a/openedx/core/lib/plugins.py +++ b/openedx/core/lib/plugins.py @@ -4,7 +4,7 @@ Adds support for first class plugins that can be added to the edX platform. from collections import OrderedDict from stevedore.extension import ExtensionManager -from openedx.core.lib.cache_utils import memoized +from openedx.core.lib.cache_utils import process_cached class PluginError(Exception): @@ -19,7 +19,7 @@ class PluginManager(object): Base class that manages plugins for the edX platform. """ @classmethod - @memoized + @process_cached def get_available_plugins(cls, namespace=None): """ Returns a dict of all the plugins that have been made available through the platform. diff --git a/common/djangoapps/util/request.py b/openedx/core/lib/request_utils.py similarity index 57% rename from common/djangoapps/util/request.py rename to openedx/core/lib/request_utils.py index 16ae251a4d9..fd69bc569c0 100644 --- a/common/djangoapps/util/request.py +++ b/openedx/core/lib/request_utils.py @@ -1,16 +1,49 @@ """ Utility functions related to HTTP requests """ import re +from urlparse import urlparse +import crum from django.conf import settings +from django.test.client import RequestFactory from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey - from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers + # accommodates course api urls, excluding any course api routes that do not fall under v*/courses, such as v1/blocks. COURSE_REGEX = re.compile(r'^(.*?/courses/)(?!v[0-9]+/[^/]+){}'.format(settings.COURSE_ID_PATTERN)) +def get_request_or_stub(): + """ + Return the current request or a stub request. + + If called outside the context of a request, construct a fake + request that can be used to build an absolute URI. + + This is useful in cases where we need to pass in a request object + but don't have an active request (for example, in tests, celery tasks, and XBlocks). + """ + request = crum.get_current_request() + + if request is None: + + # The settings SITE_NAME may contain a port number, so we need to + # parse the full URL. + full_url = "http://{site_name}".format(site_name=settings.SITE_NAME) + parsed_url = urlparse(full_url) + + # Construct the fake request. This can be used to construct absolute + # URIs to other paths. + return RequestFactory( + SERVER_NAME=parsed_url.hostname, + SERVER_PORT=parsed_url.port or 80, + ).get("/") + + else: + return request + + def safe_get_host(request): """ Get the host name for this request, as safely as possible. diff --git a/openedx/core/lib/tests/test_cache_utils.py b/openedx/core/lib/tests/test_cache_utils.py index 5179e4bc99b..e1506fac2b5 100644 --- a/openedx/core/lib/tests/test_cache_utils.py +++ b/openedx/core/lib/tests/test_cache_utils.py @@ -1,65 +1,297 @@ +# -*- coding: utf-8 -*- """ Tests for cache_utils.py """ from unittest import TestCase import ddt -from mock import MagicMock +from mock import Mock -from openedx.core.lib.cache_utils import memoize_in_request_cache +from edx_django_utils.cache import RequestCache +from openedx.core.lib.cache_utils import request_cached @ddt.ddt -class TestMemoizeInRequestCache(TestCase): +class TestRequestCachedDecorator(TestCase): """ - Test the memoize_in_request_cache helper function. + Test the request_cached decorator. """ - class TestCache(object): + def setUp(self): + RequestCache.clear_all_namespaces() + + def test_request_cached_miss_and_then_hit(self): """ - A test cache that provides a data dict for caching values, analogous to the request_cache. + Ensure that after a cache miss, we fill the cache and can hit it. """ - def __init__(self): - self.data = {} + to_be_wrapped = Mock() + to_be_wrapped.return_value = 42 + self.assertEqual(to_be_wrapped.call_count, 0) - def setUp(self): - super(TestMemoizeInRequestCache, self).setUp() - self.request_cache = self.TestCache() + def mock_wrapper(*args, **kwargs): + """Simple wrapper to let us decorate our mock.""" + return to_be_wrapped(*args, **kwargs) + + wrapped = request_cached()(mock_wrapper) + result = wrapped() + self.assertEqual(result, 42) + self.assertEqual(to_be_wrapped.call_count, 1) - @memoize_in_request_cache('request_cache') - def func_to_memoize(self, param): + result = wrapped() + self.assertEqual(result, 42) + self.assertEqual(to_be_wrapped.call_count, 1) + + def test_request_cached_with_caches_despite_changing_wrapped_result(self): """ - A test function whose results are to be memoized in the request_cache. + Ensure that after caching a result, we always send it back, even if the underlying result changes. """ - return self.func_to_count(param) + to_be_wrapped = Mock() + to_be_wrapped.side_effect = [1, 2, 3] + self.assertEqual(to_be_wrapped.call_count, 0) + + def mock_wrapper(*args, **kwargs): + """Simple wrapper to let us decorate our mock.""" + return to_be_wrapped(*args, **kwargs) + + wrapped = request_cached()(mock_wrapper) + result = wrapped() + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 1) + + result = wrapped() + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 1) - @memoize_in_request_cache('request_cache') - def multi_param_func_to_memoize(self, param1, param2): + direct_result = mock_wrapper() + self.assertEqual(direct_result, 2) + self.assertEqual(to_be_wrapped.call_count, 2) + + result = wrapped() + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 2) + + direct_result = mock_wrapper() + self.assertEqual(direct_result, 3) + self.assertEqual(to_be_wrapped.call_count, 3) + + def test_request_cached_with_changing_args(self): """ - A test function with multiple parameters whose results are to be memoized in the request_cache. + Ensure that calling a decorated function with different positional arguments + will not use a cached value invoked by a previous call with different arguments. """ - return self.func_to_count(param1, param2) + to_be_wrapped = Mock() + to_be_wrapped.side_effect = [1, 2, 3, 4, 5, 6] + self.assertEqual(to_be_wrapped.call_count, 0) + + def mock_wrapper(*args, **kwargs): + """Simple wrapper to let us decorate our mock.""" + return to_be_wrapped(*args, **kwargs) + + wrapped = request_cached()(mock_wrapper) + + # This will be a miss, and make an underlying call. + result = wrapped(1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 1) + + # This will be a miss, and make an underlying call. + result = wrapped(2) + self.assertEqual(result, 2) + self.assertEqual(to_be_wrapped.call_count, 2) - def test_memoize_in_request_cache(self): + # This is bypass of the decorator. + direct_result = mock_wrapper(3) + self.assertEqual(direct_result, 3) + self.assertEqual(to_be_wrapped.call_count, 3) + + # These will be hits, and not make an underlying call. + result = wrapped(1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 3) + + result = wrapped(2) + self.assertEqual(result, 2) + self.assertEqual(to_be_wrapped.call_count, 3) + + def test_request_cached_with_changing_kwargs(self): """ - Tests the memoize_in_request_cache decorator for both single-param and multiple-param functions. + Ensure that calling a decorated function with different keyword arguments + will not use a cached value invoked by a previous call with different arguments. """ - funcs_to_test = ( - (self.func_to_memoize, ['foo'], ['bar']), - (self.multi_param_func_to_memoize, ['foo', 'foo2'], ['foo', 'foo3']), - ) + to_be_wrapped = Mock() + to_be_wrapped.side_effect = [1, 2, 3, 4, 5, 6] + self.assertEqual(to_be_wrapped.call_count, 0) + + def mock_wrapper(*args, **kwargs): + """Simple wrapper to let us decorate our mock.""" + return to_be_wrapped(*args, **kwargs) + + wrapped = request_cached()(mock_wrapper) + + # This will be a miss, and make an underlying call. + result = wrapped(1, foo=1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 1) + + # This will be a miss, and make an underlying call. + result = wrapped(2, foo=2) + self.assertEqual(result, 2) + self.assertEqual(to_be_wrapped.call_count, 2) + + # This is bypass of the decorator. + direct_result = mock_wrapper(3, foo=3) + self.assertEqual(direct_result, 3) + self.assertEqual(to_be_wrapped.call_count, 3) + + # These will be hits, and not make an underlying call. + result = wrapped(1, foo=1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 3) + + result = wrapped(2, foo=2) + self.assertEqual(result, 2) + self.assertEqual(to_be_wrapped.call_count, 3) + + # Since we're changing foo, this will be a miss. + result = wrapped(2, foo=5) + self.assertEqual(result, 4) + self.assertEqual(to_be_wrapped.call_count, 4) + + # Since we're adding bar, this will be a miss. + result = wrapped(2, foo=1, bar=2) + self.assertEqual(result, 5) + self.assertEqual(to_be_wrapped.call_count, 5) + + # Should be a hit, even when kwargs are in a different order + result = wrapped(2, bar=2, foo=1) + self.assertEqual(result, 5) + self.assertEqual(to_be_wrapped.call_count, 5) + + def test_request_cached_mixed_unicode_str_args(self): + """ + Ensure that request_cached can work with mixed str and Unicode parameters. + """ + def dummy_function(arg1, arg2): + """ + A dummy function that expects an str and unicode arguments. + """ + assert isinstance(arg1, str), 'First parameter has to be of type `str`' + assert isinstance(arg2, unicode), 'Second parameter has to be of type `unicode`' + return True + + self.assertTrue(dummy_function('Hello', u'World'), 'Should be callable with ASCII chars') + self.assertTrue(dummy_function('H∂llå', u'Wørld'), 'Should be callable with non-ASCII chars') + + wrapped = request_cached()(dummy_function) + + self.assertTrue(wrapped('Hello', u'World'), 'Wrapper should handle ASCII only chars') + self.assertTrue(wrapped('H∂llå', u'Wørld'), 'Wrapper should handle non-ASCII chars') + + def test_request_cached_with_none_result(self): + """ + Ensure that calling a decorated function that returns None + properly caches the result and doesn't recall the underlying + function. + """ + to_be_wrapped = Mock() + to_be_wrapped.side_effect = [None, None, None, 1, 1] + self.assertEqual(to_be_wrapped.call_count, 0) + + def mock_wrapper(*args, **kwargs): + """Simple wrapper to let us decorate our mock.""" + return to_be_wrapped(*args, **kwargs) + + wrapped = request_cached()(mock_wrapper) + + # This will be a miss, and make an underlying call. + result = wrapped(1) + self.assertEqual(result, None) + self.assertEqual(to_be_wrapped.call_count, 1) + + # This will be a miss, and make an underlying call. + result = wrapped(2) + self.assertEqual(result, None) + self.assertEqual(to_be_wrapped.call_count, 2) + + # This is bypass of the decorator. + direct_result = mock_wrapper(3) + self.assertEqual(direct_result, None) + self.assertEqual(to_be_wrapped.call_count, 3) + + # These will be hits, and not make an underlying call. + result = wrapped(1) + self.assertEqual(result, None) + self.assertEqual(to_be_wrapped.call_count, 3) + + result = wrapped(2) + self.assertEqual(result, None) + self.assertEqual(to_be_wrapped.call_count, 3) + + def test_request_cached_with_request_cache_getter(self): + """ + Ensure that calling a decorated function uses + request_cache_getter if supplied. + """ + to_be_wrapped = Mock() + to_be_wrapped.side_effect = [1, 2, 3] + self.assertEqual(to_be_wrapped.call_count, 0) + + def mock_wrapper(*args, **kwargs): + """Simple wrapper to let us decorate our mock.""" + return to_be_wrapped(*args, **kwargs) + + request_cache_getter = lambda args, kwargs: RequestCache('test') + wrapped = request_cached(request_cache_getter=request_cache_getter)(mock_wrapper) + + # This will be a miss, and make an underlying call. + result = wrapped(1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 1) + + # This will be a miss, and make an underlying call. + result = wrapped(2) + self.assertEqual(result, 2) + self.assertEqual(to_be_wrapped.call_count, 2) + + # These will be a hits, and not make an underlying call. + result = wrapped(1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 2) + + # Ensure the appropriate request cache was used + self.assertFalse(RequestCache().data) + self.assertTrue(RequestCache('test').data) + + def test_request_cached_with_arg_map_function(self): + """ + Ensure that calling a decorated function uses + arg_map_function to determined the cache key. + """ + to_be_wrapped = Mock() + to_be_wrapped.side_effect = [1, 2, 3] + self.assertEqual(to_be_wrapped.call_count, 0) + + def mock_wrapper(*args, **kwargs): + """Simple wrapper to let us decorate our mock.""" + return to_be_wrapped(*args, **kwargs) - for func_to_memoize, arg_list1, arg_list2 in funcs_to_test: - self.func_to_count = MagicMock() # pylint: disable=attribute-defined-outside-init - self.assertFalse(self.func_to_count.called) + arg_map_function = lambda arg: unicode(arg == 1) + wrapped = request_cached(arg_map_function=arg_map_function)(mock_wrapper) - func_to_memoize(*arg_list1) - self.func_to_count.assert_called_once_with(*arg_list1) + # This will be a miss, and make an underlying call. + result = wrapped(1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 1) - func_to_memoize(*arg_list1) - self.func_to_count.assert_called_once_with(*arg_list1) + # This will be a miss, and make an underlying call. + result = wrapped(2) + self.assertEqual(result, 2) + self.assertEqual(to_be_wrapped.call_count, 2) - for _ in range(10): - func_to_memoize(*arg_list1) - func_to_memoize(*arg_list2) + # These will be a hits, and not make an underlying call. + result = wrapped(1) + self.assertEqual(result, 1) + self.assertEqual(to_be_wrapped.call_count, 2) - self.assertEquals(self.func_to_count.call_count, 2) + result = wrapped(3) + self.assertEqual(result, 2) + self.assertEqual(to_be_wrapped.call_count, 2) diff --git a/common/djangoapps/util/tests/test_request.py b/openedx/core/lib/tests/test_request_utils.py similarity index 81% rename from common/djangoapps/util/tests/test_request.py rename to openedx/core/lib/tests/test_request_utils.py index 4b5a129dbff..23d89f2e38f 100644 --- a/common/djangoapps/util/tests/test_request.py +++ b/openedx/core/lib/tests/test_request_utils.py @@ -1,4 +1,4 @@ -"""Tests for util.request module.""" +"""Tests for request_utils module.""" import unittest @@ -6,21 +6,32 @@ from django.conf import settings from django.core.exceptions import SuspiciousOperation from django.test.client import RequestFactory -from util.request import course_id_from_url, safe_get_host +from openedx.core.lib.request_utils import get_request_or_stub, course_id_from_url, safe_get_host -class ResponseTestCase(unittest.TestCase): - """ Tests for response-related utility functions """ +class RequestUtilTestCase(unittest.TestCase): + """ + Tests for request_utils module. + """ def setUp(self): - super(ResponseTestCase, self).setUp() + super(RequestUtilTestCase, self).setUp() self.old_site_name = settings.SITE_NAME self.old_allowed_hosts = settings.ALLOWED_HOSTS def tearDown(self): - super(ResponseTestCase, self).tearDown() + super(RequestUtilTestCase, self).tearDown() settings.SITE_NAME = self.old_site_name settings.ALLOWED_HOSTS = self.old_allowed_hosts + def test_get_request_or_stub(self): + """ + Outside the context of the request, we should still get a request + that allows us to build an absolute URI. + """ + stub = get_request_or_stub() + expected_url = "http://{site_name}/foobar".format(site_name=settings.SITE_NAME) + self.assertEqual(stub.build_absolute_uri("foobar"), expected_url) + def test_safe_get_host(self): """ Tests that the safe_get_host function returns the desired host """ settings.SITE_NAME = 'siteName.com' diff --git a/openedx/features/course_experience/utils.py b/openedx/features/course_experience/utils.py index b1a79ce8f1d..b330d538284 100644 --- a/openedx/features/course_experience/utils.py +++ b/openedx/features/course_experience/utils.py @@ -6,11 +6,11 @@ from completion.models import BlockCompletion from lms.djangoapps.course_api.blocks.api import get_blocks from lms.djangoapps.course_blocks.utils import get_student_module_as_dict from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.request_cache.middleware import request_cached +from openedx.core.lib.cache_utils import request_cached from xmodule.modulestore.django import modulestore -@request_cached +@request_cached() def get_course_outline_block_tree(request, course_id): """ Returns the root block of the course outline, with children as blocks. -- GitLab