Various improvements to the waffle classes

We expose the legacy classes in a `legacy` module, and the new ones in
`__future__`. We add monitoring to ensure that IDAs switch to the new
API.

Note that the `module_name` constructor argument is no longer optional.

We also allow the use of non-namespaced flag and switches. As per:
https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/waffle_utils/docs/decisions/0004-waffle-util-namespacing.rst
We want to be able to easily migrate existing non-namespaced
waffle.Switch and waffle.Flag objects. We avoid relying on a boolean
argument passed to the constructor, and instead implement a
`.validate_name()` method to be overridden by child classes. Using this
method, we provide NonNamespacedWaffleFlag/Switch classes and expose
them via the _future__ API.

CHANGELOG.rst

@@ -11,6 +11,16 @@ Change Log
 
 .. There should always be an "Unreleased" section for changes pending release.
 
+[1.2.0] - 2020-11-05
+~~~~~~~~~~~~~~~~~~~~
+
+* Start the deprecation process of the waffle namespace classes:
+
+  * Introduce LegacyWaffleFlag, LegacyWaffleSwitch for use with namespaces.
+  * Begin deprecation/refactoring of namespacing code, including deprecation monitoring and warnings.
+  * Note: WaffleFlag and WaffleSwitch still use namespaces as well (for now).
+  * Introduce the ``toggles.__future__`` module for applications that need to be forward-compatible right away.
+
 [1.1.1] - 2020-10-27
 ~~~~~~~~~~~~~~~~~~~~
 

edx_toggles/__init__.py

@@ -2,6 +2,6 @@
 Library and utilities for feature toggles.
 """
 
-__version__ = '1.1.1'
+__version__ = '1.2.0'
 
 default_app_config = 'edx_toggles.apps.TogglesConfig'  # pylint: disable=invalid-name

edx_toggles/tests/test_legacy_waffle.py

@@ -1,5 +1,5 @@
 """
-Unit tests for legacy waffle objects.
+Unit tests for legacy waffle objects. These tests should be moved to test_waffle.py once the legacy classes are removed.
 """
 from django.test import TestCase
 
@@ -32,19 +32,17 @@ def test_default_value(self):
         self.assertFalse(switch.is_enabled())
         self.assertFalse(namespace.is_enabled("test_switch_name"))
 
-    def test_get_set_cache_for_request(self):
+    # pylint: disable=protected-access
+    def test_set_request_cache_with_short_name(self):
         namespace = WaffleSwitchNamespace("test_namespace")
         switch = WaffleSwitch(namespace, "test_switch_name", module_name="module1")
-        self.assertFalse(
-            namespace.get_request_cache_with_short_name("test_switch_name")
-        )
+        self.assertFalse(switch._cached_switches.get("test_namespace.test_switch_name"))
         namespace.set_request_cache_with_short_name("test_switch_name", True)
-        self.assertTrue(namespace.get_request_cache_with_short_name("test_switch_name"))
+        self.assertTrue(switch._cached_switches.get("test_namespace.test_switch_name"))
         self.assertTrue(switch.is_enabled())
         namespace.set_request_cache_with_short_name("test_switch_name", False)
-        self.assertFalse(
-            namespace.get_request_cache_with_short_name("test_switch_name")
-        )
+        self.assertFalse(switch._cached_switches.get("test_namespace.test_switch_name"))
+        self.assertFalse(switch.is_enabled())
 
 
 class TestWaffleFlag(TestCase):

edx_toggles/tests/test_testutils.py

@@ -8,7 +8,9 @@
 from django.test.client import RequestFactory
 from edx_django_utils.cache import RequestCache
 
-from edx_toggles.toggles import WaffleFlag, WaffleSwitch
+# TODO import from edx_toggles.toggles once we remove the legacy classes from the exposed API
+from edx_toggles.toggles.internal.waffle.flag import WaffleFlag
+from edx_toggles.toggles.internal.waffle.switch import WaffleSwitch
 from edx_toggles.toggles.testutils import override_waffle_flag, override_waffle_switch
 
 
@@ -20,7 +22,7 @@ class OverrideWaffleFlagTests(TestCase):
     def setUp(self):
         super().setUp()
         flag_name = "test_namespace.test_flag"
-        self.waffle_flag = WaffleFlag(flag_name)
+        self.waffle_flag = WaffleFlag(flag_name, __name__)
 
         request = RequestFactory().request()
         crum.set_current_request(request)
@@ -46,8 +48,7 @@ def test_func():
     def test_override_waffle_flag_pre_cached(self):
         # checks and caches the is_enabled value
         self.assertFalse(self.waffle_flag.is_enabled())
-        # pylint: disable=protected-access
-        flag_cache = self.waffle_flag._cached_flags
+        flag_cache = self.waffle_flag.cached_flags()
         self.assertIn(self.waffle_flag.name, flag_cache)
 
         self.temporarily_enable_flag()
@@ -58,8 +59,7 @@ def test_override_waffle_flag_pre_cached(self):
 
     def test_override_waffle_flag_not_pre_cached(self):
         # check that the flag is not yet cached
-        # pylint: disable=protected-access
-        flag_cache = self.waffle_flag._cached_flags
+        flag_cache = self.waffle_flag.cached_flags()
         self.assertNotIn(self.waffle_flag.name, flag_cache)
 
         self.temporarily_enable_flag()
@@ -77,9 +77,8 @@ def test_override_waffle_flag_as_context_manager(self):
 
     def test_interlocked_overrides(self):
         waffle_flag1 = self.waffle_flag
-        waffle_flag2 = WaffleFlag(waffle_flag1.name + "2")
-        # pylint: disable=protected-access
-        waffle_flag2._cached_flags[waffle_flag2.name] = True
+        waffle_flag2 = WaffleFlag(waffle_flag1.name + "2", __name__)
+        waffle_flag2.cached_flags()[waffle_flag2.name] = True
 
         self.assertFalse(waffle_flag1.is_enabled())
         self.assertTrue(waffle_flag2.is_enabled())

edx_toggles/tests/test_waffle.py

@@ -4,7 +4,32 @@
 
 from django.test import TestCase
 
-from edx_toggles.toggles import WaffleFlag, WaffleSwitch
+from edx_toggles.toggles.internal.waffle.base import BaseWaffle
+# TODO import from edx_toggles.toggles once we remove the legacy classes from the exposed API
+from edx_toggles.toggles.internal.waffle.flag import NonNamespacedWaffleFlag, WaffleFlag
+from edx_toggles.toggles.internal.waffle.switch import NonNamespacedWaffleSwitch, WaffleSwitch
+
+
+class NaiveWaffle(BaseWaffle):
+    """
+    Simple waffle class that implements a basic instance-tracking mechanism
+    """
+    _class_instances = set()
+
+    def is_enabled(self):
+        return True
+
+
+class BaseWaffleTest(TestCase):
+    """
+    Test features of base waffle class.
+    """
+
+    def test_constructor(self):
+        waffle = NaiveWaffle("namespaced.name", "module1")
+        self.assertEqual("namespaced.name", waffle.name)
+        self.assertEqual("module1", waffle.module_name)
+        self.assertEqual(1, len(NaiveWaffle.get_instances()))
 
 
 class WaffleFlagTests(TestCase):
@@ -14,9 +39,11 @@ class WaffleFlagTests(TestCase):
 
     def test_name_validation(self):
         WaffleFlag("namespaced.name", module_name="module1")
-        self.assertRaises(
-            ValueError, WaffleFlag, "non_namespaced", module_name="module1"
-        )
+        with self.assertRaises(ValueError):
+            WaffleFlag("non_namespaced", module_name="module1")
+
+    def test_non_namespaced(self):
+        NonNamespacedWaffleFlag("non_namespaced", module_name="module1")
 
 
 class WaffleSwitchTest(TestCase):
@@ -26,10 +53,8 @@ class WaffleSwitchTest(TestCase):
 
     def test_name_validation(self):
         WaffleSwitch("namespaced.name", module_name="module1")
-        self.assertRaises(
-            ValueError, WaffleSwitch, "non_namespaced", module_name="module1"
-        )
+        with self.assertRaises(ValueError):
+            WaffleSwitch("non_namespaced", module_name="module1")
 
-    def test_constructor(self):
-        switch = WaffleSwitch("namespaced.name", module_name="module1")
-        self.assertEqual("module1", switch.module_name)
+    def test_non_namespaced(self):
+        NonNamespacedWaffleSwitch("non_namespaced", module_name="module1")

edx_toggles/toggles/__future__.py

@@ -0,0 +1,8 @@
+"""
+Expose new-style waffle classes. In the future, these imports will be moved to edx_toggles.toggles. Applications that
+want to migrate to the new-style API can do so right now by importing from this module.
+"""
+# pylint: disable=unused-import
+from .internal.setting_toggle import SettingDictToggle, SettingToggle
+from .internal.waffle.flag import NonNamespacedWaffleFlag, WaffleFlag
+from .internal.waffle.switch import NonNamespacedWaffleSwitch, WaffleSwitch

edx_toggles/toggles/__init__.py

@@ -2,9 +2,10 @@
 Expose public feature toggle API.
 """
 from .internal.setting_toggle import SettingDictToggle, SettingToggle
-from .internal.waffle.flag import WaffleFlag
-from .internal.waffle.legacy import WaffleFlag as LegacyWaffleFlag
-from .internal.waffle.legacy import WaffleFlagNamespace as LegacyWaffleFlagNamespace
-from .internal.waffle.legacy import WaffleSwitch as LegacyWaffleSwitch
-from .internal.waffle.legacy import WaffleSwitchNamespace as LegacyWaffleSwitchNamespace
-from .internal.waffle.switch import WaffleSwitch
+from .internal.waffle.legacy import WaffleFlag, WaffleFlagNamespace, WaffleSwitch, WaffleSwitchNamespace
+
+# Create waffle aliases for forward compatibility
+LegacyWaffleFlag = WaffleFlag
+LegacyWaffleFlagNamespace = WaffleFlagNamespace
+LegacyWaffleSwitch = WaffleSwitch
+LegacyWaffleSwitchNamespace = WaffleSwitchNamespace

edx_toggles/toggles/internal/base.py

@@ -15,11 +15,19 @@ class BaseToggle(ABC):
     _class_instances = None
 
     def __init__(self, name, default=False, module_name=""):
+        self.validate_name(name)
         self.name = name
         self.default = default
         self.module_name = module_name
         self._class_instances.add(self)
 
+    @classmethod
+    def validate_name(cls, name):
+        """
+        Validate the format of the instance name. This should raise a ValueError in case of incorrect format.
+        This method should only be used by child classes, mostly for overriding purposes.
+        """
+
     def is_enabled(self):
         raise NotImplementedError
 

edx_toggles/toggles/internal/waffle/__init__.py

@@ -0,0 +1,27 @@
+"""
+Waffle classes in the context of edx-platform and other IDAs.
+
+Usage:
+
+For waffle flags::
+
+    SOME_FLAG = WaffleFlag('some_namespace_prefix.some_feature', module_name=__name__)
+
+For waffle switches:
+
+    SOME_SWITCH = WaffleSwitch('some_namespace_prefix.some_feature', module_name=__name__)
+
+The namespace prefix is used to categorize waffle objects.
+
+For both flags and switched, the waffle value can be checked in code by writing::
+
+    SOME_WAFFLE.is_enabled()
+
+To test these WaffleFlags, see edx_toggles.toggles.testutils.
+
+In the above examples, you will use Django Admin "waffle" section to configure
+for a flag named: my_namespace.some_course_feature
+
+Also see ``WAFFLE_FLAG_CUSTOM_ATTRIBUTES`` and docstring for _set_waffle_flag_attribute
+for temporarily instrumenting/monitoring waffle flag usage.
+"""

edx_toggles/toggles/internal/waffle/base.py

@@ -2,6 +2,8 @@
 Base waffle toggle classes.
 """
 
+from edx_django_utils.monitoring import set_custom_attribute
+
 from ..base import BaseToggle
 
 
@@ -11,7 +13,7 @@ class BaseWaffle(BaseToggle):
     Base waffle toggle class, which performs waffle name validation.
     """
 
-    def __init__(self, name, module_name=None):
+    def __init__(self, name, module_name):
         """
         Base waffle constructor
 
@@ -20,10 +22,31 @@ def __init__(self, name, module_name=None):
             module_name (String): The name of the module where the flag is created. This should be ``__name__`` in most
             cases.
         """
+        self.validate_name(name)
+        super().__init__(name, default=False, module_name=module_name)
+        # Temporarily track usage of undefined module names
+        if not module_name:
+            set_custom_attribute(
+                "deprecated_module_not_supplied",
+                "{}[{}]".format(self.__class__.__name__, self.name),
+            )
+        # Temporarily set a custom attribute to help track usage of the legacy classes.
+        # Example: edx_toggles.toggles.internal.waffle.legacy=WaffleFlag[some.flag]
+        # TODO: Remove this custom attribute once internal.waffle.legacy has been removed.
+        set_custom_attribute(
+            self.__class__.__module__,
+            "{}[{}]".format(self.__class__.__name__, self.name),
+        )
+
+    @classmethod
+    def validate_name(cls, name):
+        """
+        Ensure that the instance name is correctly namespaced. I.e: it contains a dot (".").
+        This method should only be used by child classes, mostly for overriding purposes.
+        """
         if "." not in name:
             raise ValueError(
                 "Cannot create non-namespaced '{}' {} instance".format(
-                    name, self.__class__.__name__
+                    name, cls.__name__
                 )
             )
-        super().__init__(name, default=False, module_name=module_name)