Merge pull request #98 from uclahs-cds/aholmes-add-caching-feature-fl…

…ag-router

Abstract `FeatureFlagRouter` and add a `CachingFeatureFlagRouter`

src/platform/BL_Python/platform/feature_flag/__init__.py

@@ -0,0 +1,5 @@
+from .caching_feature_flag_router import CachingFeatureFlagRouter
+from .db_feature_flag_router import DBFeatureFlagRouter
+from .feature_flag_router import FeatureFlagRouter
+
+__all__ = ("FeatureFlagRouter", "CachingFeatureFlagRouter", "DBFeatureFlagRouter")

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

@@ -0,0 +1,81 @@
+from logging import Logger
+
+from typing_extensions import override
+
+from .feature_flag_router import FeatureFlagRouter
+
+
+class CachingFeatureFlagRouter(FeatureFlagRouter):
+    def __init__(self, logger: Logger) -> None:
+        self._logger: Logger = logger
+        self._feature_flags: dict[str, bool] = {}
+        super().__init__()
+
+    @override
+    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}`.")
+
+    def _validate_name(self, name: str):
+        if type(name) != str:
+            raise TypeError("`name` must be a string.")
+
+        if not name:
+            raise ValueError("`name` parameter is required and cannot be empty.")
+
+    @override
+    def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
+        """
+         Enables or disables a feature flag in the in-memory dictionary of feature flags.
+
+         Subclasses should call this method to validate parameters and cache values.
+
+        :param str name: The feature flag to check.
+        :param bool is_enabled: Whether the feature flag is to be enabled or disabled.
+        """
+        self._validate_name(name)
+
+        if type(is_enabled) != bool:
+            raise TypeError("`is_enabled` must be a boolean.")
+
+        self._notify_change(name, is_enabled, self._feature_flags.get(name))
+
+        self._feature_flags[name] = is_enabled
+
+        return super().set_feature_is_enabled(name, is_enabled)
+
+    @override
+    def feature_is_enabled(self, name: str, default: bool = False) -> bool:
+        """
+        Determine whether a feature flag is enabled or disabled.
+
+        Subclasses should call this method to validate parameters and use cached values.
+
+        :param str name: The feature flag to check.
+        :param bool default: If the feature flag is not in the in-memory dictionary of flags,
+            this is the default value to return. The default parameter value
+            when not specified is `False`.
+        :return bool: If `True`, the feature is enabled. If `False`, the feature is disabled.
+        """
+        self._validate_name(name)
+
+        if type(default) != bool:
+            raise TypeError("`default` must be a boolean.")
+
+        return self._feature_flags.get(name, default)
+
+    def feature_is_cached(self, name: str):
+        self._validate_name(name)
+
+        return name in self._feature_flags

src/platform/BL_Python/platform/feature_flag/db_feature_flag_router.py

@@ -1,6 +1,6 @@
 from abc import ABC
 from logging import Logger
-from typing import Type, cast
+from typing import Type, cast, overload
 
 from injector import inject
 from sqlalchemy import Boolean, Column, Unicode
@@ -9,7 +9,7 @@
 from sqlalchemy.orm.session import Session
 from typing_extensions import override
 
-from .feature_flag_router import FeatureFlagRouter
+from .caching_feature_flag_router import CachingFeatureFlagRouter
 
 
 class FeatureFlag(ABC):
@@ -56,7 +56,7 @@ def __repr__(self) -> str:
         return cast(type[FeatureFlag], _FeatureFlag)
 
 
-class DBFeatureFlagRouter(FeatureFlagRouter):
+class DBFeatureFlagRouter(CachingFeatureFlagRouter):
     _feature_flag: type[FeatureFlag]
     _session: Session
 
@@ -103,7 +103,16 @@ def set_feature_is_enabled(self, name: str, is_enabled: bool):
         self._session.commit()
         super().set_feature_is_enabled(name, is_enabled)
 
-    def feature_is_enabled(self, name: str, check_cache: bool = True) -> bool:  # type: ignore reportIncompatibleMethodOverride
+    @overload
+    def feature_is_enabled(self, name: str, default: bool = False) -> bool: ...
+    @overload
+    def feature_is_enabled(
+        self, name: str, default: bool, check_cache: bool = True
+    ) -> bool: ...
+    @override
+    def feature_is_enabled(
+        self, name: str, default: bool = False, check_cache: bool = True
+    ) -> bool:
         """
         Determine whether a feature flag is enabled or disabled.
         This method returns False if the feature flag does not exist in the database.
@@ -112,15 +121,13 @@ def feature_is_enabled(self, name: str, check_cache: bool = True) -> bool:  # ty
         for the specified feature flag. It is only cached if the value is
         pulled from the database. If the flag does not exist, no value is cached.
 
-        name: The feature flag to check.
-
-        check_cache: Whether to use the cached value if it is cached. The default is `True`.
+        :param str name: The feature flag to check.
+        :param bool default: The default value to return when a flag does not exist.
+        :param bool check_cache: Whether to use the cached value if it is cached. The default is `True`.
             If the cache is not checked, the new value pulled from the database will be cached.
         """
-        if check_cache:
-            enabled = super().feature_is_enabled(name, None)
-            if enabled is not None:
-                return enabled
+        if check_cache and super().feature_is_cached(name):
+            return super().feature_is_enabled(name, default)
 
         feature_flag = (
             self._session.query(self._feature_flag)
@@ -130,9 +137,9 @@ def feature_is_enabled(self, name: str, check_cache: bool = True) -> bool:  # ty
 
         if feature_flag is None:
             self._logger.warn(
-                f'Feature flag {name} not found in database. Returning "False" by default.'
+                f'Feature flag {name} not found in database. Returning "{default}" by default.'
             )
-            return False
+            return default
 
         is_enabled = cast(bool, feature_flag.enabled)
 

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

@@ -1,6 +1,4 @@
-from abc import ABC
-from logging import Logger
-from typing import Dict
+from abc import ABC, abstractmethod
 
 
 class FeatureFlagRouter(ABC):
@@ -9,60 +7,34 @@ class FeatureFlagRouter(ABC):
     All feature flag routers should extend this class.
     """
 
-    _logger: Logger
-    _feature_flags: Dict[str, bool]
+    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 enabled or disabled.
+        Implementation of when and whether this is called is the responsibility of subclasses.
+        This is never called by default; the base class implementation is a no-op.
 
-    def __init__(self, logger: Logger) -> None:
-        self._logger = logger
-        self._feature_flags = {}
-        super().__init__()
+        :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
     def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
         """
-        Enables or disables a feature flag in the in-memory dictionary of feature flags.
-
-        Subclasses should call this method to validate parameters and cache values.
+        Enable or disable a feature flag.
 
-        name: The feature flag to check.
-
-        is_enabled: Whether the feature flag is to be enabled or disabled.
+        :param str name: The name of the feature flag.
+        :param bool is_enabled: If `True`, the feature is enabled. If `False`, the feature is disabled.
         """
-        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]}"
-            )
-        if type(name) != str:
-            raise TypeError("`name` must be a string.")
-
-        if type(is_enabled) != bool:
-            raise TypeError("`is_enabled` must be a boolean.")
-
-        if not name:
-            raise ValueError("`name` parameter is required and cannot be empty.")
 
-        self._feature_flags[name] = is_enabled
-
-    def feature_is_enabled(
-        self, name: str, default: bool | None = False
-    ) -> bool | None:
+    @abstractmethod
+    def feature_is_enabled(self, name: str, default: bool = False) -> bool:
         """
         Determine whether a feature flag is enabled or disabled.
 
-        Subclasses should call this method to validate parameters and use cached values.
-
-        name: The feature flag to check.
-
-        default: If the feature flag is not in the in-memory dictionary of flags,
-            this is the default value to return. The default parameter value
-            when not specified is `False`.
+        :param str name: The name of the feature flag.
+        :param bool default: A default value to return for cases where a feature flag may not exist. Defaults to False.
+        :return bool: If `True`, the feature is enabled. If `False`, the feature is disabled.
         """
-        if type(name) != str:
-            raise TypeError("`name` must be a string.")
-
-        if not name:
-            raise ValueError("`name` parameter is required and cannot be empty.")
-
-        if name in self._feature_flags:
-            return self._feature_flags[name]
-
-        return default