Skip to content

fix(rc): evaluate custom signals whose value is falsy, fix default types #990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Shubham-Padkonde wants to merge 2 commits into
base: main
Choose a base branch
from
+85 −7
Open

fix(rc): evaluate custom signals whose value is falsy, fix default types #990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67a059c
fix(rc): evaluate custom signals whose value is falsy, fix default types
Shubham-Padkonde Sep 24, 2026
348de76
fix(rc): reject non-numeric custom signal values
Shubham-Padkonde Sep 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions firebase_admin/remote_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,20 +487,21 @@ def evaluate_custom_signal_condition(self, custom_signal_condition,
Returns:
True if the condition is met, False otherwise.
"""
custom_signal_operator = custom_signal_condition.get('customSignalOperator') or {}
custom_signal_key = custom_signal_condition.get('customSignalKey') or {}
custom_signal_operator = custom_signal_condition.get('customSignalOperator') or ''
custom_signal_key = custom_signal_condition.get('customSignalKey') or ''
target_custom_signal_values = (
custom_signal_condition.get('targetCustomSignalValues') or {})
custom_signal_condition.get('targetCustomSignalValues') or [])

if not all([custom_signal_operator, custom_signal_key, target_custom_signal_values]):
logger.warning("Missing operator, key, or target values for custom signal condition.")
return False

if not target_custom_signal_values:
return False
actual_custom_signal_value = context.get(custom_signal_key) or {}
actual_custom_signal_value = context.get(custom_signal_key)

if not actual_custom_signal_value:
# Falsy signal values such as 0 are valid; only a missing signal is skipped.
if actual_custom_signal_value is None:
logger.debug("Custom signal value not found in context: %s", custom_signal_key)
return False
Comment on lines +504 to 506

@gemini-code-assist gemini-code-assist Bot Sep 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

With the change to allow falsy values (such as empty lists [] or empty dictionaries {}) to pass through the None check, these values can now reach _compare_numbers when a numeric operator is evaluated.

In _compare_numbers, float(actual_value) is called. If actual_value is a list or dictionary, float() raises a TypeError (e.g., TypeError: float() argument must be a string or a real number, not 'list'). Since _compare_numbers only catches ValueError, this will cause an unhandled exception and crash the evaluation.

To prevent this, _compare_numbers should be updated to catch both ValueError and TypeError:

def _compare_numbers(self, custom_signal_key, target_value, actual_value, predicate_fn) -> bool:
    try:
        target = float(target_value)
        actual = float(actual_value)
        result = -1 if actual < target else 1 if actual > target else 0
        return predicate_fn(result)
    except (ValueError, TypeError):
        logger.warning('Invalid numeric value for comparison for custom signal key %s.', custom_signal_key)
        return False

@Shubham-Padkonde Shubham-Padkonde Sep 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 348de76: numeric comparisons now reject both ValueError and TypeError. Added empty-list and empty-dictionary cases for all six numeric operators: all 12 failed before the fix and now pass; the full Remote Config module passes all 47 tests. Source/test pylint and git diff --check pass. Prepared with Codex assistance.


Expand Down Expand Up @@ -613,7 +614,7 @@ def _compare_numbers(self, custom_signal_key, target_value, actual_value, predic
actual = float(actual_value)
result = -1 if actual < target else 1 if actual > target else 0
return predicate_fn(result)
except ValueError:
except (ValueError, TypeError):
logger.warning("Invalid numeric value for comparison for custom signal key %s.",
custom_signal_key)
return False
Expand Down Expand Up @@ -739,7 +740,7 @@ def as_boolean(self) -> bool:
return self.DEFAULT_VALUE_FOR_BOOLEAN
return str(self.value).lower() in self.BOOLEAN_TRUTHY_VALUES

def as_int(self) -> float:
def as_int(self) -> int:
"""Returns the value as a number."""
if self.source == 'static':
return self.DEFAULT_VALUE_FOR_INTEGER
Expand Down
77 changes: 77 additions & 0 deletions tests/test_remote_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,6 +820,83 @@ def test_evaluate_custom_signal_semantic_version(self,
assert server_config.get_boolean('is_enabled') == parameter_value


@pytest.mark.parametrize(
'custom_signal_operator, target_custom_signal_value, context_value, parameter_value',
[
(CustomSignalOperator.NUMERIC_EQUAL.value, ['0'], 0, True),
(CustomSignalOperator.NUMERIC_LESS_THAN.value, ['1'], 0, True),
(CustomSignalOperator.NUMERIC_GREATER_THAN.value, ['-1'], 0.0, True),
(CustomSignalOperator.NUMERIC_GREATER_THAN.value, ['0'], 0, False),
(CustomSignalOperator.STRING_EXACTLY_MATCHES.value, ['0'], 0, True),
])
def test_evaluate_custom_signal_zero_value(self,
custom_signal_operator,
target_custom_signal_value,
context_value,
parameter_value):
server_template = self._custom_signal_template(custom_signal_operator,
target_custom_signal_value)
context = {'randomization_id': '123', 'signal_key': context_value}
server_config = server_template.evaluate(context)
assert server_config.get_boolean('is_enabled') == parameter_value

@pytest.mark.parametrize('context_value', [[], {}])
@pytest.mark.parametrize('operator', [
CustomSignalOperator.NUMERIC_LESS_THAN,
CustomSignalOperator.NUMERIC_LESS_EQUAL,
CustomSignalOperator.NUMERIC_EQUAL,
CustomSignalOperator.NUMERIC_NOT_EQUAL,
CustomSignalOperator.NUMERIC_GREATER_THAN,
CustomSignalOperator.NUMERIC_GREATER_EQUAL,
])
def test_evaluate_custom_signal_non_numeric_value(self, context_value, operator):
server_template = self._custom_signal_template(operator.value, ['0'])
server_config = server_template.evaluate({'signal_key': context_value})
assert server_config.get_boolean('is_enabled') is False

def test_evaluate_custom_signal_missing_value(self):
server_template = self._custom_signal_template(
CustomSignalOperator.NUMERIC_LESS_THAN.value, ['1'])
server_config = server_template.evaluate({'randomization_id': '123'})
assert server_config.get_boolean('is_enabled') is False
Comment on lines +857 to +861

@gemini-code-assist gemini-code-assist Bot Sep 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It would be highly beneficial to add test cases where the custom signal value in the context is an empty list [] or an empty dictionary {}. This will ensure that these falsy, non-numeric types are handled gracefully without raising a TypeError during numeric comparisons.


def _custom_signal_template(self, custom_signal_operator, target_custom_signal_value):
condition = {
'name': 'is_true',
'condition': {
'orCondition': {
'conditions': [{
'andCondition': {
'conditions': [{
'customSignal': {
'customSignalOperator': custom_signal_operator,
'customSignalKey': 'signal_key',
'targetCustomSignalValues': target_custom_signal_value
}
}],
}
}]
}
}
}
template_data = {
'conditions': [condition],
'parameters': {
'is_enabled': {
'defaultValue': {'value': 'false'},
'conditionalValues': {'is_true': {'value': 'true'}}
},
},
'parameterGroups': '',
'version': '',
'etag': '123'
}
return remote_config.init_server_template(
app=firebase_admin.get_app(),
default_config={'dog_is_cute': True},
template_data_json=json.dumps(template_data)
)

class MockAdapter(testutils.MockAdapter):
"""A Mock HTTP Adapter that provides Firebase Remote Config responses with ETag in header."""

Expand Down
Loading