From aff61120311a164bd556d8f40e474ff6abbbe994 Mon Sep 17 00:00:00 2001 From: Aaron Holmes Date: Thu, 8 Aug 2024 15:54:06 -0700 Subject: [PATCH] Add a `default` parameter to Database feature flags `is_enabled`. This fixes an issue with an inconsistent interface where `check_cache` and `default` collide. Related #98 --- .../caching_feature_flag_router.py | 3 +++ .../feature_flag/db_feature_flag_router.py | 25 +++++++++++++------ .../test_db_feature_flag_router.py | 14 +++++++---- 3 files changed, 29 insertions(+), 13 deletions(-) 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 index 9fd5bc5b..0c7a21a9 100644 --- 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 @@ -70,6 +70,9 @@ def feature_is_enabled(self, name: str, default: bool = False) -> bool: """ 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): 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 9d3f9f79..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 @@ -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,13 +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 and super().feature_is_cached(name): - return super().feature_is_enabled(name) + return super().feature_is_enabled(name, default) feature_flag = ( self._session.query(self._feature_flag) @@ -128,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/test/unit/feature_flags/test_db_feature_flag_router.py b/src/platform/test/unit/feature_flags/test_db_feature_flag_router.py index a44ae26d..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): @@ -187,7 +191,7 @@ def test__feature_is_enabled__checks_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 feature_is_enabled_mock.call_count == check_cache[1] @@ -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]