From 3a29cff0169cbe6dd9f59fa0cb0fcbab5ebbb836 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Fri, 8 Jan 2021 18:32:29 +0100 Subject: [PATCH] Get rid of calls to `set_request_cache_with_short_name` This method from the toggle legacy classes should not actually be exposed to all. So we get rid of it by manually setting the cached value. While we are at it, we convert the STORAGE_BACKING_FOR_CACHE legacy waffle switch to its modern version. As the flag is not being used elsewhere, this should not break anything. We take the opportunity to modernize waffle switches from block_structure.config: to do so we convert the INVALIDATE_CACHE_ON_PUBLISH and RAISE_ERROR_WHEN_NOT_FOUND waffle switches from legacy classes to their modern equivalents. These switches are not used outside of edx-platform, so this change should not trigger any error. --- .../course_api/blocks/tests/test_api.py | 6 +-- .../block_structure/config/__init__.py | 37 ++++++++----------- .../commands/generate_course_blocks.py | 5 +-- .../content/block_structure/manager.py | 5 +-- .../content/block_structure/signals.py | 2 +- .../content/block_structure/store.py | 31 ++++++---------- .../content/block_structure/tasks.py | 6 +-- .../block_structure/tests/test_manager.py | 6 +-- .../block_structure/tests/test_signals.py | 4 +- .../block_structure/tests/test_store.py | 10 ++--- .../commands/tests/test_dump_to_neo4j.py | 4 +- 11 files changed, 50 insertions(+), 66 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index f738fd09bf..65f7097cad 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -17,7 +17,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import SampleCourseFactory, check_mongo_calls from xmodule.modulestore.tests.sample_courses import BlockInfo from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache -from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle_switch +from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE from ..api import get_blocks @@ -230,7 +230,7 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): ) @ddt.unpack def test_query_counts_cached(self, store_type, with_storage_backing): - with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing): + with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): course = self._create_course(store_type) self._get_blocks( course, @@ -247,7 +247,7 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): @ddt.unpack def test_query_counts_uncached(self, store_type_tuple, with_storage_backing): store_type, expected_mongo_queries = store_type_tuple - with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing): + with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): course = self._create_course(store_type) clear_course_from_cache(course.id) diff --git a/openedx/core/djangoapps/content/block_structure/config/__init__.py b/openedx/core/djangoapps/content/block_structure/config/__init__.py index 8657512d25..754c2b17be 100644 --- a/openedx/core/djangoapps/content/block_structure/config/__init__.py +++ b/openedx/core/djangoapps/content/block_structure/config/__init__.py @@ -2,37 +2,32 @@ This module contains various configuration settings via waffle switches for the Block Structure framework. """ +from edx_django_utils.cache import RequestCache +from edx_toggles.toggles.__future__ import WaffleSwitch - -from edx_toggles.toggles import LegacyWaffleSwitch, LegacyWaffleSwitchNamespace from openedx.core.lib.cache_utils import request_cached from .models import BlockStructureConfiguration -# Namespace -WAFFLE_NAMESPACE = u'block_structure' - # Switches -INVALIDATE_CACHE_ON_PUBLISH = u'invalidate_cache_on_publish' -STORAGE_BACKING_FOR_CACHE = u'storage_backing_for_cache' -RAISE_ERROR_WHEN_NOT_FOUND = u'raise_error_when_not_found' +INVALIDATE_CACHE_ON_PUBLISH = WaffleSwitch( + "block_structure.invalidate_cache_on_publish", __name__ +) +STORAGE_BACKING_FOR_CACHE = WaffleSwitch( + "block_structure.storage_backing_for_cache", __name__ +) +RAISE_ERROR_WHEN_NOT_FOUND = WaffleSwitch( + "block_structure.raise_error_when_not_found", __name__ +) -def waffle(): +def enable_storage_backing_for_cache_in_request(): """ - Returns the namespaced and cached Waffle class for BlockStructures. + Manually override the value of the STORAGE_BACKING_FOR_CACHE switch in the context of the request. + This function should not be replicated, as it accesses a protected member, and it shouldn't. """ - return LegacyWaffleSwitchNamespace(name=WAFFLE_NAMESPACE, log_prefix=u'BlockStructure: ') - - -def waffle_switch(name): - """ - Return the waffle switch associated to this namespace. - - WARNING: do not replicate this pattern. Instead of declaring waffle switch names as strings, you should create - LegacyWaffleSwitch objects as top-level constants. - """ - return LegacyWaffleSwitch(waffle(), name, module_name=__name__) + # pylint: disable=protected-access + STORAGE_BACKING_FOR_CACHE._cached_switches[STORAGE_BACKING_FOR_CACHE.name] = True @request_cached() diff --git a/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py b/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py index 1ee189a528..45f50d72cf 100644 --- a/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py +++ b/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py @@ -7,13 +7,12 @@ import logging import six from django.core.management.base import BaseCommand -import six from six import text_type import openedx.core.djangoapps.content.block_structure.api as api import openedx.core.djangoapps.content.block_structure.store as store import openedx.core.djangoapps.content.block_structure.tasks as tasks -from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle +from openedx.core.djangoapps.content.block_structure.config import enable_storage_backing_for_cache_in_request from openedx.core.lib.command_utils import ( get_mutually_exclusive_required_option, parse_course_keys, @@ -133,7 +132,7 @@ class Command(BaseCommand): Generates course blocks for the given course_keys per the given options. """ if options.get('with_storage'): - waffle().set_request_cache_with_short_name(STORAGE_BACKING_FOR_CACHE, True) + enable_storage_backing_for_cache_in_request() for course_key in course_keys: try: diff --git a/openedx/core/djangoapps/content/block_structure/manager.py b/openedx/core/djangoapps/content/block_structure/manager.py index 1623d208c2..63aa2fe3a6 100644 --- a/openedx/core/djangoapps/content/block_structure/manager.py +++ b/openedx/core/djangoapps/content/block_structure/manager.py @@ -102,10 +102,9 @@ class BlockStructureManager(object): BlockStructureTransformers.verify_versions(block_structure) except (BlockStructureNotFound, TransformerDataIncompatible): - if config.waffle().is_enabled(config.RAISE_ERROR_WHEN_NOT_FOUND): + if config.RAISE_ERROR_WHEN_NOT_FOUND.is_enabled(): raise - else: - block_structure = self._update_collected() + block_structure = self._update_collected() return block_structure diff --git a/openedx/core/djangoapps/content/block_structure/signals.py b/openedx/core/djangoapps/content/block_structure/signals.py index 43fe00113a..1e01fc7a13 100644 --- a/openedx/core/djangoapps/content/block_structure/signals.py +++ b/openedx/core/djangoapps/content/block_structure/signals.py @@ -29,7 +29,7 @@ def update_block_structure_on_course_publish(sender, course_key, **kwargs): # p if isinstance(course_key, LibraryLocator): return - if config.waffle().is_enabled(config.INVALIDATE_CACHE_ON_PUBLISH): + if config.INVALIDATE_CACHE_ON_PUBLISH.is_enabled(): try: clear_course_from_cache(course_key) except BlockStructureNotFound: diff --git a/openedx/core/djangoapps/content/block_structure/store.py b/openedx/core/djangoapps/content/block_structure/store.py index 28e76dc4a0..df1a23144b 100644 --- a/openedx/core/djangoapps/content/block_structure/store.py +++ b/openedx/core/djangoapps/content/block_structure/store.py @@ -7,8 +7,8 @@ Module for the Storage of BlockStructure objects. from logging import getLogger import six - from django.utils.encoding import python_2_unicode_compatible + from openedx.core.lib.cache_utils import zpickle, zunpickle from . import config @@ -28,6 +28,7 @@ class StubModel(object): By using this stub, we eliminate the need for extra conditional statements in the code. """ + def __init__(self, root_block_usage_key): self.data_usage_key = root_block_usage_key @@ -45,6 +46,7 @@ class BlockStructureStore(object): """ Storage for BlockStructure objects. """ + def __init__(self, cache): """ Arguments: @@ -121,7 +123,7 @@ class BlockStructureStore(object): Returns whether the data in storage for the given key is already up-to-date with the version in the given modulestore. """ - if _is_storage_backing_enabled(): + if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): try: bs_model = self._get_model(root_block_usage_key) root_block = modulestore.get_item(root_block_usage_key) @@ -135,7 +137,7 @@ class BlockStructureStore(object): """ Returns the model associated with the given key. """ - if _is_storage_backing_enabled(): + if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): return BlockStructureModel.get(root_block_usage_key) else: return StubModel(root_block_usage_key) @@ -145,7 +147,7 @@ class BlockStructureStore(object): Updates or creates the model for the given block_structure and serialized_data. """ - if _is_storage_backing_enabled(): + if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): root_block = block_structure[block_structure.root_block_usage_key] bs_model, _ = BlockStructureModel.update_or_create( serialized_data, @@ -187,7 +189,7 @@ class BlockStructureStore(object): Raises: BlockStructureNotFound if not found. """ - if not _is_storage_backing_enabled(): + if not config.STORAGE_BACKING_FOR_CACHE.is_enabled(): raise BlockStructureNotFound(bs_model.data_usage_key) return bs_model.get_serialized_data() @@ -229,14 +231,12 @@ class BlockStructureStore(object): Returns the cache key to use for the given BlockStructureModel or StubModel. """ - if _is_storage_backing_enabled(): + if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): return six.text_type(bs_model) - - else: - return "v{version}.root.key.{root_usage_key}".format( - version=six.text_type(BlockStructureBlockData.VERSION), - root_usage_key=six.text_type(bs_model.data_usage_key), - ) + return "v{version}.root.key.{root_usage_key}".format( + version=six.text_type(BlockStructureBlockData.VERSION), + root_usage_key=six.text_type(bs_model.data_usage_key), + ) @staticmethod def _version_data_of_block(root_block): @@ -260,10 +260,3 @@ class BlockStructureStore(object): field_name: getattr(bs_model, field_name, None) for field_name in BlockStructureModel.VERSION_FIELDS } - - -def _is_storage_backing_enabled(): - """ - Returns whether storage backing for Block Structures is enabled. - """ - return config.waffle().is_enabled(config.STORAGE_BACKING_FOR_CACHE) diff --git a/openedx/core/djangoapps/content/block_structure/tasks.py b/openedx/core/djangoapps/content/block_structure/tasks.py index 50f8519a2a..970f48e4cd 100644 --- a/openedx/core/djangoapps/content/block_structure/tasks.py +++ b/openedx/core/djangoapps/content/block_structure/tasks.py @@ -14,7 +14,7 @@ from opaque_keys.edx.keys import CourseKey from capa.responsetypes import LoncapaProblemError from openedx.core.djangoapps.content.block_structure import api -from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle +from openedx.core.djangoapps.content.block_structure.config import enable_storage_backing_for_cache_in_request from xmodule.modulestore.exceptions import ItemNotFoundError log = logging.getLogger('edx.celery.task') @@ -63,7 +63,7 @@ def _update_course_in_cache(self, **kwargs): Updates the course blocks (mongo -> BlockStructure) for the specified course. """ if kwargs.get('with_storage'): - waffle().set_request_cache_with_short_name(STORAGE_BACKING_FOR_CACHE, True) + enable_storage_backing_for_cache_in_request() _call_and_retry_if_needed(self, api.update_course_in_cache, **kwargs) @@ -94,7 +94,7 @@ def _get_course_in_cache(self, **kwargs): Gets the course blocks for the specified course, updating the cache if needed. """ if kwargs.get('with_storage'): - waffle().set_request_cache_with_short_name(STORAGE_BACKING_FOR_CACHE, True) + enable_storage_backing_for_cache_in_request() _call_and_retry_if_needed(self, api.get_course_in_cache, **kwargs) diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py index cfdf68fb1a..1c0d921426 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py @@ -9,7 +9,7 @@ from django.test import TestCase from edx_toggles.toggles.testutils import override_waffle_switch from ..block_structure import BlockStructureBlockData -from ..config import RAISE_ERROR_WHEN_NOT_FOUND, STORAGE_BACKING_FOR_CACHE, waffle_switch +from ..config import RAISE_ERROR_WHEN_NOT_FOUND, STORAGE_BACKING_FOR_CACHE from ..exceptions import BlockStructureNotFound, UsageKeyNotInBlockStructure from ..manager import BlockStructureManager from ..transformers import BlockStructureTransformers @@ -179,14 +179,14 @@ class TestBlockStructureManager(UsageKeyFactoryMixin, ChildrenMapTestMixin, Test assert TestTransformer1.collect_call_count == 1 def test_get_collected_error_raised(self): - with override_waffle_switch(waffle_switch(RAISE_ERROR_WHEN_NOT_FOUND), active=True): + with override_waffle_switch(RAISE_ERROR_WHEN_NOT_FOUND, active=True): with mock_registered_transformers(self.registered_transformers): with self.assertRaises(BlockStructureNotFound): self.bs_manager.get_collected() @ddt.data(True, False) def test_update_collected_if_needed(self, with_storage_backing): - with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing): + with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): with mock_registered_transformers(self.registered_transformers): assert TestTransformer1.collect_call_count == 0 diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_signals.py b/openedx/core/djangoapps/content/block_structure/tests/test_signals.py index ca47fb2eeb..99e3db6a61 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_signals.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_signals.py @@ -12,7 +12,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from ..api import get_block_structure_manager -from ..config import INVALIDATE_CACHE_ON_PUBLISH, waffle_switch +from ..config import INVALIDATE_CACHE_ON_PUBLISH from ..signals import update_block_structure_on_course_publish from .helpers import is_course_in_block_structure_cache @@ -56,7 +56,7 @@ class CourseBlocksSignalTest(ModuleStoreTestCase): def test_cache_invalidation(self, invalidate_cache_enabled, mock_bs_manager_clear): test_display_name = "Jedi 101" - with override_waffle_switch(waffle_switch(INVALIDATE_CACHE_ON_PUBLISH), active=invalidate_cache_enabled): + with override_waffle_switch(INVALIDATE_CACHE_ON_PUBLISH, active=invalidate_cache_enabled): self.course.display_name = test_display_name self.store.update_item(self.course, self.user.id) diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_store.py b/openedx/core/djangoapps/content/block_structure/tests/test_store.py index 85772578db..ef35a70139 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_store.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_store.py @@ -8,7 +8,7 @@ from edx_toggles.toggles.testutils import override_waffle_switch from openedx.core.djangolib.testing.utils import CacheIsolationTestCase -from ..config import STORAGE_BACKING_FOR_CACHE, waffle_switch +from ..config import STORAGE_BACKING_FOR_CACHE from ..config.models import BlockStructureConfiguration from ..exceptions import BlockStructureNotFound from ..store import BlockStructureStore @@ -48,13 +48,13 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI @ddt.data(True, False) def test_get_none(self, with_storage_backing): - with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing): + with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): with self.assertRaises(BlockStructureNotFound): self.store.get(self.block_structure.root_block_usage_key) @ddt.data(True, False) def test_add_and_get(self, with_storage_backing): - with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing): + with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): self.store.add(self.block_structure) stored_value = self.store.get(self.block_structure.root_block_usage_key) self.assertIsNotNone(stored_value) @@ -62,7 +62,7 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI @ddt.data(True, False) def test_delete(self, with_storage_backing): - with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing): + with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): self.store.add(self.block_structure) self.store.delete(self.block_structure.root_block_usage_key) with self.assertRaises(BlockStructureNotFound): @@ -75,7 +75,7 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI self.store.get(self.block_structure.root_block_usage_key) def test_uncached_with_storage(self): - with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=True): + with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=True): self.store.add(self.block_structure) self.mock_cache.map.clear() stored_value = self.store.get(self.block_structure.root_block_usage_key) diff --git a/openedx/core/djangoapps/coursegraph/management/commands/tests/test_dump_to_neo4j.py b/openedx/core/djangoapps/coursegraph/management/commands/tests/test_dump_to_neo4j.py index 7431a90392..febacf0819 100644 --- a/openedx/core/djangoapps/coursegraph/management/commands/tests/test_dump_to_neo4j.py +++ b/openedx/core/djangoapps/coursegraph/management/commands/tests/test_dump_to_neo4j.py @@ -500,9 +500,7 @@ class TestModuleStoreSerializer(TestDumpToNeo4jCommandBase): self.assertEqual(len(submitted), len(self.course_strings)) # simulate one of the courses being published - with override_waffle_switch( - block_structure_config.waffle_switch(block_structure_config.STORAGE_BACKING_FOR_CACHE), True - ): + with override_waffle_switch(block_structure_config.STORAGE_BACKING_FOR_CACHE, True): update_block_structure_on_course_publish(None, self.course.id) # make sure only the published course was dumped