Change how notifications work for feature flags.

Due to an oversight in what Python allows, the notification methods for
feature flag changes was made abstract, requiring subclasses to
implement it. However, this was intended to be optional because the
feature flag base class is meant to only require implementations for the
get/set interface. To resolve this, the `abstractmethod` decoration has
been removed.

Another change here regards nonsensical notifications in
`CachingFeatureFlagRouter`. The previous notification logged any time
`set` was called, and always said that the flag changed from its
current value to its current value (that is not a typo). In addition
to fixing this, the notification now indicates:

* When an attempt was made to set a flag to its current value
* When a flag was set, and what its new value is
* When a new flag is set and cached

Related #98

src/platform/BL_Python/platform/feature_flag/caching_feature_flag_router.py

@@ -11,21 +11,21 @@ def __init__(self, logger: Logger) -> None:
         self._feature_flags: dict[str, bool] = {}
         super().__init__()
 
-    def _notify_changed(self, name: str):
-        if name in self._feature_flags:
-            self._logger.warn(
-                f"Overridding feature flag value for '{name}'. Toggling from {self._feature_flags[name]} to {self._feature_flags[name]}"
-            )
-
     @override
-    def _notify_enabled(self, name: str):
-        self._notify_changed(name)
-        super()._notify_enabled(name)
-
-    @override
-    def _notify_disabled(self, name: str):
-        self._notify_changed(name)
-        super()._notify_disabled(name)
+    def _notify_change(
+        self, name: str, new_value: bool, old_value: bool | None
+    ) -> None:
+        if name in self._feature_flags:
+            if new_value == old_value:
+                self._logger.warn(
+                    f"Tried to change feature flag value for '{name}' to the same value. It is already {'enabled' if new_value else 'disabled'}."
+                )
+            else:
+                self._logger.warn(
+                    f"Changing feature flag value for '{name}' from `{old_value}` to `{new_value}`."
+                )
+        else:
+            self._logger.warn(f"Setting new feature flag '{name}' to `{new_value}`.")
 
     @override
     def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
@@ -46,7 +46,7 @@ def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
         if not name:
             raise ValueError("`name` parameter is required and cannot be empty.")
 
-        self._notify_changed(name)
+        self._notify_change(name, is_enabled, self._feature_flags.get(name))
 
         self._feature_flags[name] = is_enabled
 

src/platform/BL_Python/platform/feature_flag/feature_flag_router.py

@@ -7,24 +7,17 @@ class FeatureFlagRouter(ABC):
     All feature flag routers should extend this class.
     """
 
-    @abstractmethod
-    def _notify_enabled(self, name: str) -> None:
-        """
-        Override to provide a method to be used to notify when a feature is enabled.
-        Implementation of when and whether this is called is the responsibility of subclasses.
-        This is never called by default.
-
-        :param str name: The name of the feature flag.
-        """
-
-    @abstractmethod
-    def _notify_disabled(self, name: str) -> None:
+    def _notify_change(
+        self, name: str, new_value: bool, old_value: bool | None
+    ) -> None:
         """
-        Override to provide a method to be used to notify when a feature is disabled.
+        Override to provide a method to be used to notify when a feature is enabled or disabled.
         Implementation of when and whether this is called is the responsibility of subclasses.
-        This is never called by default.
+        This is never called by default; the base class implementation is a no-op.
 
         :param str name: The name of the feature flag.
+        :param bool new_value: The value that the flag is changing to.
+        :param bool | None old_value: The value that the flag is changing from.
         """
 
     @abstractmethod

src/platform/test/unit/feature_flags/test_caching_feature_flag_router.py

@@ -0,0 +1,134 @@
+import logging
+from typing import Any
+
+import pytest
+from BL_Python.platform.feature_flag.caching_feature_flag_router import (
+    CachingFeatureFlagRouter,
+)
+from typing_extensions import override
+
+_FEATURE_FLAG_TEST_NAME = "foo_feature"
+
+
+def test__feature_is_enabled__disallows_empty_name():
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    with pytest.raises(ValueError):
+        _ = caching_feature_flag_router.feature_is_enabled("")
+
+
+@pytest.mark.parametrize("name", [0, False, True, {}, [], (0,)])
+def test__feature_is_enabled__disallows_non_string_names(name: Any):
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    with pytest.raises(TypeError):
+        _ = caching_feature_flag_router.feature_is_enabled(name)
+
+
+def test__set_feature_is_enabled__disallows_empty_name():
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    with pytest.raises(ValueError):
+        caching_feature_flag_router.set_feature_is_enabled("", False)
+
+
+@pytest.mark.parametrize("name", [0, False, True, {}, [], (0,)])
+def test__set_feature_is_enabled__disallows_non_string_names(name: Any):
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    with pytest.raises(TypeError):
+        caching_feature_flag_router.set_feature_is_enabled(name, False)
+
+
+@pytest.mark.parametrize("value", [None, "", "False", "True", 0, 1, -1, {}, [], (0,)])
+def test__set_feature_is_enabled__disallows_non_bool_values(value: Any):
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    with pytest.raises(TypeError) as e:
+        _ = caching_feature_flag_router.set_feature_is_enabled(
+            _FEATURE_FLAG_TEST_NAME, value
+        )
+
+    assert e.match("`is_enabled` must be a boolean")
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test__set_feature_is_enabled__sets_correct_value(value: bool):
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    caching_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, value)
+    is_enabled = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+
+    assert is_enabled == value
+
+
+def test__feature_is_enabled__defaults_to_false_when_flag_does_not_exist():
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    is_enabled = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+
+    assert is_enabled == False
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test__set_feature_is_enabled__caches_new_flag(value: bool):
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    caching_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, value)
+    assert _FEATURE_FLAG_TEST_NAME in caching_feature_flag_router._feature_flags  # pyright: ignore[reportPrivateUsage]
+    is_enabled = caching_feature_flag_router._feature_flags.get(_FEATURE_FLAG_TEST_NAME)  # pyright: ignore[reportPrivateUsage]
+    assert is_enabled == value
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test__feature_is_enabled__uses_cache(value: bool):
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    call_count = 0
+
+    class FakeDict(dict):  # pyright: ignore[reportMissingTypeArgument]
+        @override
+        def __getitem__(self, key: Any) -> Any:
+            nonlocal call_count
+            call_count += 1
+            return super().__getitem__(key)  # pyright: ignore[reportUnknownMemberType,reportUnknownVariableType]
+
+    caching_feature_flag_router._feature_flags = FakeDict()  # pyright: ignore[reportPrivateUsage]
+
+    caching_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, value)
+    _ = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+    _ = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+
+    assert call_count == 2
+
+
+@pytest.mark.parametrize("enable", [True, False])
+def test__set_feature_is_enabled__resets_cache_when_flag_enable_is_set(enable: bool):
+    logger = logging.getLogger("FeatureFlagLogger")
+    caching_feature_flag_router = CachingFeatureFlagRouter(logger)
+
+    caching_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, enable)
+    _ = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+    first_value = caching_feature_flag_router.feature_is_enabled(
+        _FEATURE_FLAG_TEST_NAME
+    )
+
+    caching_feature_flag_router.set_feature_is_enabled(
+        _FEATURE_FLAG_TEST_NAME, not enable
+    )
+    _ = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+    second_value = caching_feature_flag_router.feature_is_enabled(
+        _FEATURE_FLAG_TEST_NAME
+    )
+
+    assert first_value == enable
+    assert second_value == (not enable)

src/platform/test/unit/feature_flags/test_feature_flag_router.py

@@ -0,0 +1,73 @@
+import ast
+import inspect
+
+from BL_Python.platform.feature_flag.feature_flag_router import FeatureFlagRouter
+from typing_extensions import override
+
+_FEATURE_FLAG_TEST_NAME = "foo_feature"
+
+
+class TestFeatureFlagRouter(FeatureFlagRouter):
+    @override
+    def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
+        return super().set_feature_is_enabled(name, is_enabled)
+
+    @override
+    def feature_is_enabled(
+        self, name: str, default: bool | None = False
+    ) -> bool | None:
+        return super().feature_is_enabled(name, default)
+
+
+class NotifyingFeatureFlagRouter(TestFeatureFlagRouter):
+    def __init__(self) -> None:
+        self.notification_count = 0
+        super().__init__()
+
+    @override
+    def _notify_change(
+        self, name: str, new_value: bool, old_value: bool | None
+    ) -> None:
+        self.notification_count += 1
+        return super()._notify_change(name, new_value, old_value)
+
+
+def test___notify_change__is_a_noop():
+    test_feature_flag_router = TestFeatureFlagRouter()
+    source = inspect.getsource(test_feature_flag_router._notify_change)  # pyright: ignore[reportPrivateUsage]
+    # strip the leading \t because the method is in a class
+    # and the AST parse will fail because of unexpected indent
+    parsed = ast.parse(source.strip())
+    function_node = parsed.body[0]
+    # [0] is the docblock
+    function_body_nodes = function_node.body[1:]  # pyright: ignore[reportUnknownMemberType,reportUnknownVariableType,reportAttributeAccessIssue]
+
+    # empty means the function doesn't contain any code
+    assert not function_body_nodes
+
+
+def test__set_feature_is_enabled__does_not_notify():
+    notifying_feature_flag_router = NotifyingFeatureFlagRouter()
+
+    notifying_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, True)
+
+    assert notifying_feature_flag_router.notification_count == 0
+
+
+def test__feature_is_enabled__does_not_notify():
+    notifying_feature_flag_router = NotifyingFeatureFlagRouter()
+
+    _ = notifying_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+
+    assert notifying_feature_flag_router.notification_count == 0
+
+
+def test__feature_is_enabled__does_not_notify_after_flag_is_set():
+    notifying_feature_flag_router = NotifyingFeatureFlagRouter()
+
+    notifying_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, True)
+    notifying_feature_flag_router.notification_count = 0
+
+    _ = notifying_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
+
+    assert notifying_feature_flag_router.notification_count == 0