Skip to content

Fix RemoteConfig.all returning defaults instead of fetched values on Apple - #848

Open
anggrayudi wants to merge 1 commit into
GitLiveApp:masterfrom
anggrayudi:fix/427-remote-config-all-precedence
Open

Fix RemoteConfig.all returning defaults instead of fetched values on Apple#848
anggrayudi wants to merge 1 commit into
GitLiveApp:masterfrom
anggrayudi:fix/427-remote-config-all-precedence

Conversation

@anggrayudi

Copy link
Copy Markdown

Fixes #427.

Problem

On Apple targets, FirebaseRemoteConfig.all read every key once per source and merged the results:

return listOf(
    FIRRemoteConfigSource.FIRRemoteConfigSourceStatic,
    FIRRemoteConfigSource.FIRRemoteConfigSourceRemote,
    FIRRemoteConfigSource.FIRRemoteConfigSourceDefault,   // <- last
).map { source ->
    val keys = ios.allKeysFromSource(source) as List<String>
    keys.map { it to FirebaseRemoteConfigValue(ios.configValueForKey(it, source)) }
}.flatten().toMap()

Two things combine badly here:

  1. each value is fetched pinned to a specific source via configValueForKey(key, source), so it bypasses Remote Config's own precedence; and
  2. toMap() keeps the last pair for a duplicate key, and Default is listed last.

So for any key present in both the remote and default sources, all returns the default, shadowing the value that was just fetched.

That matches the report exactly: with setDefaults(...) the reporter saw stale defaults out of all even though the logs showed a successful fetch and activate, and removing setDefaults(...) made the remote values appear — because with no defaults registered there is no colliding key to overwrite them.

The documented precedence is remote > default > static, so this is backwards.

Apple was the only target with the problem:

  • JS already does the right thing: getAllKeys().associateWith { getValue(it) }
  • Android delegates to the native all, which applies precedence itself

Fix

Collect the key set across the three sources, then resolve each key through configValueForKey(key) — the single-argument overload the class already uses in getValue() — so Firebase applies its own precedence. This is the same shape as the JS implementation.

getKeysByPrefix() is implemented on top of all, so it was affected the same way and is fixed by the same change.

On testing

I could not run the build or the tests locally — Gradle dependency resolution stalls in my environment and I never got a green or red run, the same as I noted on #847. Please treat CI as the real check.

More importantly, this is not something the existing suite could have caught, and I want to be explicit about why rather than imply otherwise. testGetAll only calls setDefaults(...) and never fetches, so every key comes from the default source alone — there is no collision, and the test passes identically before and after this change. A genuine regression test needs a key that exists in both the remote and default sources, which means real remote values; that is exactly why testFetch and testFetchAndActivate are @Ignored, since Remote Config isn't covered by the Firebase emulator.

So rather than add a test that cannot fail, I extended the existing @Ignored testFetchAndActivate to register a default under the same key as the remote value and assert that all agrees with getValue. It won't run in CI, but it encodes the regression and makes it verifiable by anyone running that test manually against a project with the console value set.

Happy to take a different approach if you'd prefer the precedence pinned by an explicitly ordered merge instead of delegating to configValueForKey(key).

`all` read every key once per source and merged the results with `toMap()`.
Because `Default` was listed last and `toMap()` keeps the last pair for a
duplicate key, a key present in both the remote and default sources resolved
to the default, shadowing the fetched remote value. Callers who had called
setDefaults() therefore saw stale defaults from `all` even after a successful
fetchAndActivate(), while dropping setDefaults() made the remote values
appear.

Collect the key set across sources instead, then resolve each key through
configValueForKey(key) so Firebase applies its own precedence
(remote > default > static). This matches the JS target, which already does
getAllKeys().associateWith { getValue(it) }, and Android, which delegates to
the native `all`.

getKeysByPrefix() is built on `all` and was affected the same way.

Fixes GitLiveApp#427
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.

iOS RemoteConfig - fetchAndActivate doesn't work with setDefaults.

1 participant