Repository navigation
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
Conversation
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.
There was a problem hiding this comment.
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.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
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.A conditional value or default value entry without a
valuefield (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.