diff --git a/openedx/core/djangoapps/waffle_utils/__init__.py b/openedx/core/djangoapps/waffle_utils/__init__.py index 140fa8a074..36a741a949 100644 --- a/openedx/core/djangoapps/waffle_utils/__init__.py +++ b/openedx/core/djangoapps/waffle_utils/__init__.py @@ -52,7 +52,7 @@ Also see ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` and docstring for _set_waffle_flag_at for temporarily instrumenting/monitoring waffle flag usage. """ - +from functools import lru_cache import logging import re import traceback @@ -61,7 +61,6 @@ from contextlib import contextmanager from weakref import WeakSet import crum -import six from django.conf import settings from edx_django_utils.monitoring import set_custom_attribute from opaque_keys.edx.keys import CourseKey @@ -76,29 +75,31 @@ from edx_toggles.toggles import WaffleSwitchNamespace as BaseWaffleSwitchNamespa log = logging.getLogger(__name__) -class RequestCacheMixin: - @staticmethod - def _get_request_cache(): - """ - Returns a request cache shared by all instances of this class. - """ - return get_request_cache("WaffleNamespace") +class WaffleSwitchNamespace(BaseWaffleSwitchNamespace): + """ + A waffle switch namespace class that implements request-based caching. + """ - -class WaffleSwitchNamespace(BaseWaffleSwitchNamespace, RequestCacheMixin): @property def _cached_switches(self): """ Returns a dictionary of all namespaced switches in the request cache. """ - return self._get_request_cache().setdefault("switches", {}) + return _get_waffle_namespace_request_cache().setdefault("switches", {}) + + +def _get_waffle_namespace_request_cache(): + """ + Returns a request cache shared by all Waffle namespace objects. + """ + return get_request_cache("WaffleNamespace") class WaffleSwitch(BaseWaffleSwitch): NAMESPACE_CLASS = WaffleSwitchNamespace -class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin): +class WaffleFlagNamespace(BaseNamespace): """ Provides a single namespace for a set of waffle flags. @@ -111,20 +112,13 @@ class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin): """ Returns a dictionary of all namespaced flags in the request cache. """ - return self._get_request_cache().setdefault("flags", {}) + return _get_waffle_namespace_request_cache().setdefault("flags", {}) - def is_flag_active(self, flag_name, check_before_waffle_callback=None): + def is_flag_active(self, flag_name): """ Returns and caches whether the provided flag is active. If the flag value is already cached in the request, it is returned. - If check_before_waffle_callback is supplied, it is called before - checking waffle. - If check_before_waffle_callback returns None, or if it is not supplied, - then waffle is used to check the flag. - - Important: Caching for the check_before_waffle_callback must be handled - by the callback itself. Note: A waffle flag's default is False if not defined. If you think you need the default to be True, see the module docstring for @@ -132,121 +126,50 @@ class WaffleFlagNamespace(BaseNamespace, RequestCacheMixin): Arguments: flag_name (String): The name of the flag to check. - check_before_waffle_callback (function): (Optional) A function that - will be checked before continuing on to waffle. If - check_before_waffle_callback(namespaced_flag_name) returns True - or False, it is returned. If it returns None, then waffle is - used. - """ - # validate arguments namespaced_flag_name = self._namespaced_name(flag_name) - - if check_before_waffle_callback: - value = check_before_waffle_callback(namespaced_flag_name) - if value is not None: - # Do not cache value for the callback, because the key might be different. - # The callback needs to handle its own caching if it wants it. - self._set_waffle_flag_attribute(namespaced_flag_name, value) - return value - - value = self._cached_flags.get(namespaced_flag_name) - if value is not None: - self._set_waffle_flag_attribute(namespaced_flag_name, value) - return value - - request = crum.get_current_request() - if not request: - log.warning( - u"%sFlag '%s' accessed without a request", - self.log_prefix, - namespaced_flag_name, - ) - # Return the Flag's Everyone value if not in a request context. - # Note: this skips the cache as the value might be different - # in a normal request context. This case seems to occur when - # a page redirects to a 404, or for celery workers. - value = self._is_flag_active_for_everyone(namespaced_flag_name) - self._set_waffle_flag_attribute(namespaced_flag_name, value) - set_custom_attribute("warn_flag_no_request_return_value", value) - return value - - value = flag_is_active(request, namespaced_flag_name) - self._cached_flags[namespaced_flag_name] = value - - self._set_waffle_flag_attribute(namespaced_flag_name, value) + value = self._get_flag_active(namespaced_flag_name) + _set_waffle_flag_attribute(namespaced_flag_name, value) return value - def _is_flag_active_for_everyone(self, namespaced_flag_name): + def _get_flag_active(self, namespaced_flag_name): """ - Returns True if the waffle flag is configured as active for Everyone, - False otherwise. + Return the value of the flag activation without touching the _waffle_flag_attribute. """ - # Import is placed here to avoid model import at project startup. - from waffle.models import Flag + # Check namespace cache + value = self._cached_flags.get(namespaced_flag_name) + if value is not None: + return value - try: - waffle_flag = Flag.objects.get(name=namespaced_flag_name) - return waffle_flag.everyone is True - except Flag.DoesNotExist: - return False + # Check in context of request + request = crum.get_current_request() + value = self._get_flag_active_request(namespaced_flag_name, request) + if value is not None: + return value - def _set_waffle_flag_attribute(self, name, value): + # Check value in the absence of any context + log.warning( + u"%sFlag '%s' accessed without a request", + self.log_prefix, + namespaced_flag_name, + ) + # Return the Flag's Everyone value if not in a request context. + # Note: this skips the cache as the value might be different + # in a normal request context. This case seems to occur when + # a page redirects to a 404, or for celery workers. + value = is_flag_active_for_everyone(namespaced_flag_name) + set_custom_attribute("warn_flag_no_request_return_value", value) + return value + + def _get_flag_active_request(self, namespaced_flag_name, request): """ - For any flag name in _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET, add name/value - to cached values and set custom attribute if the value changed. - - The name of the custom attribute will have the prefix ``flag_`` and the - suffix will match the name of the flag. - The value of the custom attribute could be False, True, or Both. - - The value Both would mean that the flag had both a True and False - value at different times during the transaction. This is most - likely due to having a check_before_waffle_callback, as is the - case with CourseWaffleFlag. - - An example NewRelic query to see the values of a flag in different - environments, if your waffle flag was named ``my.waffle.flag`` might - look like:: - - SELECT count(*) FROM Transaction - WHERE flag_my.waffle.flag IS NOT NULL - FACET appName, flag_my.waffle.flag - - Important: Remember to configure ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` for - LMS, Studio and Workers in order to see waffle flag usage in all - edx-platform environments. - + Get flag value in the context of the current request. """ - if name not in _WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET: - return - - flag_attribute_data = self._get_request_cache().setdefault("flag_attribute", {}) - is_value_change = False - if name not in flag_attribute_data: - flag_attribute_data[name] = str(value) - is_value_change = True - else: - if flag_attribute_data[name] != str(value): - flag_attribute_data[name] = "Both" - is_value_change = True - - if is_value_change: - attribute_name = "flag_{}".format(name) - set_custom_attribute(attribute_name, flag_attribute_data[name]) - - -def _get_waffle_flag_custom_attributes_set(): - """ - Returns a set based on the Django setting WAFFLE_FLAG_CUSTOM_ATTRIBUTES (list). - """ - waffle_flag_custom_attributes = getattr( - settings, _WAFFLE_FLAG_CUSTOM_ATTRIBUTES, None - ) - waffle_flag_custom_attributes = ( - waffle_flag_custom_attributes if waffle_flag_custom_attributes else [] - ) - return set(waffle_flag_custom_attributes) + if request: + value = flag_is_active(request, namespaced_flag_name) + self._cached_flags[namespaced_flag_name] = value + return value + return None # .. toggle_name: WAFFLE_FLAG_CUSTOM_ATTRIBUTES @@ -257,16 +180,86 @@ def _get_waffle_flag_custom_attributes_set(): # .. toggle_use_cases: opt_in # .. toggle_creation_date: 2020-06-17 # .. toggle_warnings: Intent is for temporary research (1 day - several weeks) of a flag's usage. -_WAFFLE_FLAG_CUSTOM_ATTRIBUTES = "WAFFLE_FLAG_CUSTOM_ATTRIBUTES" - -# set of waffle flags that should be instrumented with custom attributes -_WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET = _get_waffle_flag_custom_attributes_set() +@lru_cache(maxsize=None) +def _get_waffle_flag_custom_attributes_set(): + attributes = getattr(settings, "WAFFLE_FLAG_CUSTOM_ATTRIBUTES", None) or [] + return set(attributes) -class WaffleFlag(object): +def _set_waffle_flag_attribute(name, value): """ - Represents a single waffle flag, using a cached waffle namespace. + For any flag name in settings.WAFFLE_FLAG_CUSTOM_ATTRIBUTES, add name/value + to cached values and set custom attribute if the value changed. + + The name of the custom attribute will have the prefix ``flag_`` and the + suffix will match the name of the flag. + The value of the custom attribute could be False, True, or Both. + + The value Both would mean that the flag had both a True and False + value at different times during the transaction. This is most + likely due to having a course override, as is the case with + CourseWaffleFlag. + + An example NewRelic query to see the values of a flag in different + environments, if your waffle flag was named ``my.waffle.flag`` might + look like:: + + SELECT count(*) FROM Transaction + WHERE flag_my.waffle.flag IS NOT NULL + FACET appName, flag_my.waffle.flag + + Important: Remember to configure ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` for + LMS, Studio and Workers in order to see waffle flag usage in all + edx-platform environments. """ + if name not in _get_waffle_flag_custom_attributes_set(): + return + + flag_attribute_data = _get_waffle_namespace_request_cache().setdefault( + "flag_attribute", {} + ) + is_value_changed = True + if name not in flag_attribute_data: + # New flag + flag_attribute_data[name] = str(value) + else: + # Existing flag + if flag_attribute_data[name] == str(value): + # Same value + is_value_changed = False + else: + # New value + flag_attribute_data[name] = "Both" + + if is_value_changed: + attribute_name = "flag_{}".format(name) + set_custom_attribute(attribute_name, flag_attribute_data[name]) + + +def is_flag_active_for_everyone(namespaced_flag_name): + """ + Returns True if the waffle flag is configured as active for Everyone, + False otherwise. + """ + # Import is placed here to avoid model import at project startup. + from waffle.models import Flag + + try: + waffle_flag = Flag.objects.get(name=namespaced_flag_name) + return waffle_flag.everyone is True + except Flag.DoesNotExist: + return False + + +class WaffleFlag: + """ + Waffle flag class that implements custom override method. + + This class should be removed in favour of edx_toggles.toggles.WaffleFlag once we get rid of the WaffleFlagNamespace + class and the `override` method. + """ + + NAMESPACE_CLASS = WaffleFlagNamespace # use a WeakSet so these instances can be garbage collected if need be _class_instances = WeakSet() @@ -281,10 +274,9 @@ class WaffleFlag(object): module_name (String): The name of the module where the flag is created. This should be ``__name__`` in most cases. """ - if isinstance(waffle_namespace, six.string_types): - waffle_namespace = WaffleFlagNamespace(name=waffle_namespace) + if isinstance(waffle_namespace, str): + waffle_namespace = self.NAMESPACE_CLASS(name=waffle_namespace) - self.waffle_namespace = waffle_namespace self.waffle_namespace = waffle_namespace self.flag_name = flag_name self._module_name = module_name or "" @@ -311,6 +303,7 @@ class WaffleFlag(object): """ Returns the fully namespaced flag name. """ + # pylint: disable=protected-access return self.waffle_namespace._namespaced_name(self.flag_name) def is_enabled(self): @@ -321,6 +314,9 @@ class WaffleFlag(object): @contextmanager def override(self, active=True): + """ + Shortcut method for `override_waffle_flag`. + """ # TODO We can move this import to the top of the file once this code is # not all contained within the __init__ module. from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag @@ -334,47 +330,49 @@ class CourseWaffleFlag(WaffleFlag): Represents a single waffle flag that can be forced on/off for a course. Uses a cached waffle namespace. + + Usage: + + WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='my_namespace') + SOME_COURSE_FLAG = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'some_course_feature', __name__) + + And then we can check this flag in code with:: + + SOME_COURSE_FLAG.is_enabled(course_key) + + The Django Admin "waffle_utils" section can be used to configure a course override for this same flag (e.g. + my_namespace.some_course_feature). """ - def _get_course_override_callback(self, course_key): + def _get_course_override_value(self, course_key): """ - Returns a function to use as the check_before_waffle_callback. + Returns True/False if the flag was forced on or off for the provided course. Returns None if the flag was not + overridden. + + Note: Has side effect of caching the override value. Arguments: - course_key (CourseKey): The course to check for override before - checking waffle. + course_key (CourseKey): The course to check for override before checking waffle. """ + # Import is placed here to avoid model import at project startup. + from .models import WaffleFlagCourseOverrideModel - def course_override_callback(namespaced_flag_name): - """ - Returns True/False if the flag was forced on or off for the provided - course. Returns None if the flag was not overridden. + cache_key = "{}.{}".format(self.namespaced_flag_name, str(course_key)) + # pylint: disable=protected-access + course_override = self.waffle_namespace._cached_flags.get(cache_key) - Note: Has side effect of caching the override value. + if course_override is None: + course_override = WaffleFlagCourseOverrideModel.override_value( + self.namespaced_flag_name, course_key + ) + # pylint: disable=protected-access + self.waffle_namespace._cached_flags[cache_key] = course_override - Arguments: - namespaced_flag_name (String): A namespaced version of the flag - to check. - """ - # Import is placed here to avoid model import at project startup. - from .models import WaffleFlagCourseOverrideModel - - cache_key = u"{}.{}".format(namespaced_flag_name, six.text_type(course_key)) - force_override = self.waffle_namespace._cached_flags.get(cache_key) - - if force_override is None: - force_override = WaffleFlagCourseOverrideModel.override_value( - namespaced_flag_name, course_key - ) - self.waffle_namespace._cached_flags[cache_key] = force_override - - if force_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.on: - return True - if force_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.off: - return False - return None - - return course_override_callback + if course_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.on: + return True + if course_override == WaffleFlagCourseOverrideModel.ALL_CHOICES.off: + return False + return None def is_enabled(self, course_key=None): # pylint: disable=arguments-differ """ @@ -391,7 +389,8 @@ class CourseWaffleFlag(WaffleFlag): ), "Provided course_key '{}' is not instance of CourseKey.".format( course_key ) - return self.waffle_namespace.is_flag_active( - self.flag_name, - check_before_waffle_callback=self._get_course_override_callback(course_key), - ) + is_enabled_for_course = self._get_course_override_value(course_key) + if is_enabled_for_course is not None: + _set_waffle_flag_attribute(self.namespaced_flag_name, is_enabled_for_course) + return is_enabled_for_course + return super().is_enabled() diff --git a/openedx/core/djangoapps/waffle_utils/tests/test_init.py b/openedx/core/djangoapps/waffle_utils/tests/test_init.py index 2d69635a81..fa32c647b0 100644 --- a/openedx/core/djangoapps/waffle_utils/tests/test_init.py +++ b/openedx/core/djangoapps/waffle_utils/tests/test_init.py @@ -57,35 +57,33 @@ class TestCourseWaffleFlag(TestCase): Tests various combinations of a flag being set in waffle and overridden for a course. """ - with patch( - 'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET', - _get_waffle_flag_custom_attributes_set(), - ): - with patch.object(WaffleFlagCourseOverrideModel, 'override_value', return_value=data['course_override']): - with override_flag(self.NAMESPACED_FLAG_NAME, active=data['waffle_enabled']): - # check twice to test that the result is properly cached - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) - # result is cached, so override check should happen once - WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( - self.NAMESPACED_FLAG_NAME, - self.TEST_COURSE_KEY - ) + _get_waffle_flag_custom_attributes_set.cache_clear() + with patch.object(WaffleFlagCourseOverrideModel, 'override_value', return_value=data['course_override']): + with override_flag(self.NAMESPACED_FLAG_NAME, active=data['waffle_enabled']): + # check twice to test that the result is properly cached + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_KEY), data['result']) + # result is cached, so override check should happen once + # pylint: disable=no-member + WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( + self.NAMESPACED_FLAG_NAME, + self.TEST_COURSE_KEY + ) - self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=str(data['result'])) - mock_set_custom_attribute.reset_mock() + self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=str(data['result'])) + mock_set_custom_attribute.reset_mock() - # check flag for a second course - if data['course_override'] == WaffleFlagCourseOverrideModel.ALL_CHOICES.unset: - # When course override wasn't set for the first course, the second course will get the same - # cached value from waffle. - second_value = data['waffle_enabled'] - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) - else: - # When course override was set for the first course, it should not apply to the second - # course which should get the default value of False. - second_value = False - self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) + # check flag for a second course + if data['course_override'] == WaffleFlagCourseOverrideModel.ALL_CHOICES.unset: + # When course override wasn't set for the first course, the second course will get the same + # cached value from waffle. + second_value = data['waffle_enabled'] + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) + else: + # When course override was set for the first course, it should not apply to the second + # course which should get the default value of False. + second_value = False + self.assertEqual(self.TEST_COURSE_FLAG.is_enabled(self.TEST_COURSE_2_KEY), second_value) expected_flag_value = None if second_value == data['result'] else 'Both' self._assert_waffle_flag_attribute(mock_set_custom_attribute, expected_flag_value=expected_flag_value) @@ -102,23 +100,21 @@ class TestCourseWaffleFlag(TestCase): __name__, ) - with patch( - 'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET', - _get_waffle_flag_custom_attributes_set(), + _get_waffle_flag_custom_attributes_set.cache_clear() + with patch.object( + WaffleFlagCourseOverrideModel, + 'override_value', + return_value=WaffleFlagCourseOverrideModel.ALL_CHOICES.unset ): - with patch.object( - WaffleFlagCourseOverrideModel, - 'override_value', - return_value=WaffleFlagCourseOverrideModel.ALL_CHOICES.unset - ): - # check twice to test that the result is properly cached - self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) - self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) - # result is cached, so override check should happen once - WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( - self.NAMESPACED_FLAG_NAME, - self.TEST_COURSE_KEY - ) + # check twice to test that the result is properly cached + self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) + self.assertEqual(test_course_flag.is_enabled(self.TEST_COURSE_KEY), False) + # result is cached, so override check should happen once + # pylint: disable=no-member + WaffleFlagCourseOverrideModel.override_value.assert_called_once_with( + self.NAMESPACED_FLAG_NAME, + self.TEST_COURSE_KEY + ) self._assert_waffle_flag_attribute( mock_set_custom_attribute, @@ -161,14 +157,11 @@ class TestCourseWaffleFlag(TestCase): Test that custom attributes are recorded when waffle flag accessed. """ with override_settings(WAFFLE_FLAG_CUSTOM_ATTRIBUTES=data['waffle_flag_attribute_setting']): - with patch( - 'openedx.core.djangoapps.waffle_utils._WAFFLE_FLAG_CUSTOM_ATTRIBUTE_SET', - _get_waffle_flag_custom_attributes_set(), - ): - test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME, __name__) - test_course_flag.is_enabled(self.TEST_COURSE_KEY) - test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME, __name__) - test_course_flag_2.is_enabled(self.TEST_COURSE_KEY) + _get_waffle_flag_custom_attributes_set.cache_clear() + test_course_flag = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_NAME, __name__) + test_course_flag.is_enabled(self.TEST_COURSE_KEY) + test_course_flag_2 = CourseWaffleFlag(self.TEST_NAMESPACE, self.FLAG_2_NAME, __name__) + test_course_flag_2.is_enabled(self.TEST_COURSE_KEY) self.assertEqual(mock_set_custom_attribute.call_count, data['expected_count']) diff --git a/openedx/features/course_experience/__init__.py b/openedx/features/course_experience/__init__.py index 5ae92ba88c..40be566219 100644 --- a/openedx/features/course_experience/__init__.py +++ b/openedx/features/course_experience/__init__.py @@ -8,7 +8,8 @@ from waffle import flag_is_active from lms.djangoapps.experiments.flags import ExperimentWaffleFlag from openedx.core.djangoapps.util.user_messages import UserMessageCollection -from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag, WaffleFlag, WaffleFlagNamespace +from openedx.core.djangoapps.waffle_utils import WaffleFlag, WaffleFlagNamespace +from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag # Namespace for course experience waffle flags. WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='course_experience')