Skip to content

Commit

Permalink
Add a default parameter to Database feature flags is_enabled.
Browse files Browse the repository at this point in the history
This fixes an issue with an inconsistent interface where `check_cache`
and `default` collide.

Related #98
  • Loading branch information
aholmes committed Aug 8, 2024
1 parent 5a374e7 commit aff6112
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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)
Expand All @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,21 @@ 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")
db_feature_flag_router = DBFeatureFlagRouter(
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):
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand Down

0 comments on commit aff6112

Please sign in to comment.