Skip to content

fix(remoteconfig): compare custom signals whose value is 0 or empty; don't build values from a missing parameter value - #994

Open
rootkiller6788 wants to merge 4 commits into
firebase:mainfrom
rootkiller6788:fix-remote-config-signal-value
Open

rootkiller6788 wants to merge 4 commits into
firebase:mainfrom
rootkiller6788:fix-remote-config-signal-value

Conversation

@rootkiller6788

Copy link
Copy Markdown

Two things in remote_config.py don't line up with how the same evaluator behaves in firebase-admin-node, plus a type annotation that is plain wrong.

  1. A custom signal whose value is 0 or '' is treated as if the signal were not in the context at all, because it is read with context.get(key) or {} and then checked for truthiness. So {"user_level": 0} can never match NUMERIC_EQUAL with target '0'. The context is typed Dict[str, Union[str, int]], so 0 is a perfectly valid signal value. The signal is now only considered missing when the context has no entry for it, same as the Node SDK.

  2. A conditional value or default value entry without a value field (for example {"useInAppDefault": false}) produced a Value backed by None, so get_string() handed back the literal string 'None' and get_int() blew up with a TypeError out of int(None). Conditional values without an explicit value now leave the in-app default in place, and a Value never holds None.

I also corrected the as_int() return annotation, which is int and not float - that part comes from #957 (the other half of that issue, the or {} fallbacks for the string/list fields, is covered by the change above).

Testing: new unit tests in tests/test_remote_config.py; all of them fail on main. The module suite is green (36 passed) and pylint is clean on the changed files.

The signal pulled out of the evaluation context was coerced with `or {}`,
so any falsy value (0, '') was treated as an absent signal and the whole
condition returned false. Context values are declared as Union[str, int],
so 0 is a valid signal and a NUMERIC_EQUAL '0' condition could never match.

Only treat the signal as missing when the context has no entry for it,
matching firebase-admin-node.
A parameter whose conditional value or default value entry has no `value`
field (for example `{"useInAppDefault": false}`) produced a Value backed by
None. get_string() then returned the literal string 'None' and get_int()
raised a TypeError out of int(None).

Skip conditional values without an explicit value so the in-app default
stays in place, and never store None in a Value.
as_int() always returns an int, not a float.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request improves Remote Config evaluation logic by handling conditional values without explicit values, ensuring falsy custom signal values (like 0 or empty strings) are correctly evaluated instead of treated as absent, and fixing type annotations and fallback values. It also adds corresponding unit tests. The feedback highlights a critical issue in the new tests where mutating the global SERVER_REMOTE_CONFIG_RESPONSE dictionary directly can cause test pollution; using .copy() is recommended to avoid flaky tests.

Comment thread tests/test_remote_config.py Resolved
Comment thread tests/test_remote_config.py Resolved
@rootkiller6788
rootkiller6788 marked this pull request as ready for review October 12, 2026 15:47
@rootkiller6788
rootkiller6788 requested a review from a team October 12, 2026 15:47
Copy SERVER_REMOTE_CONFIG_RESPONSE before overriding its conditions and
parameters in evaluate() tests. The previous tests mutated the module-level
dictionary in place, leaking state into later tests and making the suite
order-dependent.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants