diff --git a/src/platform/BL_Python/platform/feature_flag/__init__.py b/src/platform/BL_Python/platform/feature_flag/__init__.py index e69de29b..082c8fe8 100644 --- a/src/platform/BL_Python/platform/feature_flag/__init__.py +++ b/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") diff --git a/src/platform/BL_Python/platform/feature_flag/caching_feature_flag_router.py b/src/platform/BL_Python/platform/feature_flag/caching_feature_flag_router.py new file mode 100644 index 00000000..0c7a21a9 --- /dev/null +++ b/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 diff --git a/src/platform/BL_Python/platform/feature_flag/db_feature_flag_router.py b/src/platform/BL_Python/platform/feature_flag/db_feature_flag_router.py index d1069336..b7fabbe7 100644 --- a/src/platform/BL_Python/platform/feature_flag/db_feature_flag_router.py +++ b/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) diff --git a/src/platform/BL_Python/platform/feature_flag/feature_flag_router.py b/src/platform/BL_Python/platform/feature_flag/feature_flag_router.py index 8ce23c17..d66ac562 100644 --- a/src/platform/BL_Python/platform/feature_flag/feature_flag_router.py +++ b/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 diff --git a/src/platform/test/unit/feature_flags/test_caching_feature_flag_router.py b/src/platform/test/unit/feature_flags/test_caching_feature_flag_router.py new file mode 100644 index 00000000..94aa1278 --- /dev/null +++ b/src/platform/test/unit/feature_flags/test_caching_feature_flag_router.py @@ -0,0 +1,186 @@ +import logging +from typing import Any + +import pytest +from BL_Python.platform.feature_flag.caching_feature_flag_router import ( + CachingFeatureFlagRouter, +) +from mock import MagicMock +from pytest import LogCaptureFixture + +_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) + + mock_dict = MagicMock() + caching_feature_flag_router._feature_flags = mock_dict # 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 mock_dict.get.call_count == 3 + + +@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) + + +@pytest.mark.parametrize("value", [True, False]) +def test__set_feature_is_enabled__notifies_when_setting_new_flag( + value: bool, + caplog: LogCaptureFixture, +): + logger = logging.getLogger("FeatureFlagLogger") + caching_feature_flag_router = CachingFeatureFlagRouter(logger) + + caching_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, value) + + assert f"Setting new feature flag '{_FEATURE_FLAG_TEST_NAME}' to `{value}`." in { + record.msg for record in caplog.records + } + + +@pytest.mark.parametrize( + "first_value,second_value,expected_log_msg", + [ + [ + True, + True, + f"Tried to change feature flag value for '{_FEATURE_FLAG_TEST_NAME}' to the same value. It is already enabled.", + ], + [ + False, + False, + f"Tried to change feature flag value for '{_FEATURE_FLAG_TEST_NAME}' to the same value. It is already disabled.", + ], + [ + True, + False, + f"Changing feature flag value for '{_FEATURE_FLAG_TEST_NAME}' from `True` to `False`.", + ], + [ + False, + True, + f"Changing feature flag value for '{_FEATURE_FLAG_TEST_NAME}' from `False` to `True`.", + ], + ], +) +def test__set_feature_is_enabled__notifies_when_changing_flag( + first_value: bool, + second_value: bool, + expected_log_msg: str, + caplog: LogCaptureFixture, +): + logger = logging.getLogger("FeatureFlagLogger") + caching_feature_flag_router = CachingFeatureFlagRouter(logger) + + caching_feature_flag_router.set_feature_is_enabled( + _FEATURE_FLAG_TEST_NAME, first_value + ) + caching_feature_flag_router.set_feature_is_enabled( + _FEATURE_FLAG_TEST_NAME, second_value + ) + + assert expected_log_msg in {record.msg for record in caplog.records} diff --git a/src/platform/test/unit/feature_flags/test_db_feature_flag_router.py b/src/platform/test/unit/feature_flags/test_db_feature_flag_router.py index ec58659a..11ac6ce0 100644 --- a/src/platform/test/unit/feature_flags/test_db_feature_flag_router.py +++ b/src/platform/test/unit/feature_flags/test_db_feature_flag_router.py @@ -67,7 +67,9 @@ def test__feature_is_enabled__defaults_to_false(feature_flag_session: Session): assert is_enabled == False -def test__feature_is_enabled__defaults_to_false_when_flag_does_not_exist( +@pytest.mark.parametrize("default", [True, False]) +def test__feature_is_enabled__uses_default_when_flag_does_not_exist( + default: bool, feature_flag_session: Session, ): logger = logging.getLogger("FeatureFlagLogger") @@ -75,9 +77,11 @@ def test__feature_is_enabled__defaults_to_false_when_flag_does_not_exist( FeatureFlag, feature_flag_session, logger ) - is_enabled = db_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME) + is_enabled = db_feature_flag_router.feature_is_enabled( + _FEATURE_FLAG_TEST_NAME, default + ) - assert is_enabled == False + assert is_enabled == default def test__feature_is_enabled__disallows_empty_name(feature_flag_session: Session): @@ -177,17 +181,17 @@ def test__feature_is_enabled__checks_cache( ): session_mock = mocker.patch("sqlalchemy.orm.session.Session") feature_is_enabled_mock = mocker.patch( - "BL_Python.platform.feature_flag.feature_flag_router.FeatureFlagRouter.feature_is_enabled" + "BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.feature_is_cached" ) _ = mocker.patch( - "BL_Python.platform.feature_flag.feature_flag_router.FeatureFlagRouter.set_feature_is_enabled" + "BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.set_feature_is_enabled" ) logger = logging.getLogger("FeatureFlagLogger") db_feature_flag_router = DBFeatureFlagRouter(FeatureFlag, session_mock, logger) _ = db_feature_flag_router.feature_is_enabled( - _FEATURE_FLAG_TEST_NAME, check_cache[0] + _FEATURE_FLAG_TEST_NAME, False, check_cache[0] ) assert feature_is_enabled_mock.call_count == check_cache[1] @@ -199,10 +203,10 @@ def test__feature_is_enabled__sets_cache( ): session_mock = mocker.patch("sqlalchemy.orm.session.Session") feature_is_enabled_mock = mocker.patch( - "BL_Python.platform.feature_flag.feature_flag_router.FeatureFlagRouter.feature_is_enabled" + "BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.feature_is_cached" ) set_feature_is_enabled_mock = mocker.patch( - "BL_Python.platform.feature_flag.feature_flag_router.FeatureFlagRouter.set_feature_is_enabled" + "BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.set_feature_is_enabled" ) feature_is_enabled_mock.return_value = True @@ -210,7 +214,7 @@ def test__feature_is_enabled__sets_cache( db_feature_flag_router = DBFeatureFlagRouter(FeatureFlag, session_mock, logger) _ = db_feature_flag_router.feature_is_enabled( - _FEATURE_FLAG_TEST_NAME, check_cache[0] + _FEATURE_FLAG_TEST_NAME, False, check_cache[0] ) assert set_feature_is_enabled_mock.call_count == check_cache[1] diff --git a/src/platform/test/unit/feature_flags/test_feature_flag_router.py b/src/platform/test/unit/feature_flags/test_feature_flag_router.py new file mode 100644 index 00000000..589548bd --- /dev/null +++ b/src/platform/test/unit/feature_flags/test_feature_flag_router.py @@ -0,0 +1,71 @@ +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 = False) -> bool: + 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