fix(android,remote_config): ensureInitialized() task should ignore return value (fixes #5222) #5467
Conversation
…turn value (fixes #5222) Currently on Android the task returned by RemoteConfig.ensureInitialized() returns a `FirebaseRemoteConfigInfo` instance which gets sent to the Flutter standard message codec and causes a crash as this isn't a supported value that can be serialized by the codec. As we don't at this point care about the value of the task result, I've wrapped then task in a `whenAll` which will just return void when completed.
| int fetchTimeout = Objects.requireNonNull(call.argument("fetchTimeout")); | ||
| int minimumFetchInterval = Objects.requireNonNull(call.argument("minimumFetchInterval")); |
There was a problem hiding this comment.
Never null as these come from Dart where they are required values.
| @@ -144,7 +145,7 @@ public void onMethodCall(MethodCall call, final MethodChannel.Result result) { | |||
| } | |||
| case "RemoteConfig#setDefaults": | |||
| { | |||
| Map<String, Object> defaults = call.argument("defaults"); | |||
| Map<String, Object> defaults = Objects.requireNonNull(call.argument("defaults")); | |||
There was a problem hiding this comment.
Never null as this comes from Dart where it is a required value.
| case FirebaseRemoteConfig.LAST_FETCH_STATUS_THROTTLED: | ||
| return "throttled"; | ||
| case FirebaseRemoteConfig.LAST_FETCH_STATUS_NO_FETCH_YET: | ||
| return "noFetchYet"; | ||
| case FirebaseRemoteConfig.LAST_FETCH_STATUS_FAILURE: | ||
| default: |
There was a problem hiding this comment.
Duplicate return cases, default and failure return the same thing
| case FirebaseRemoteConfig.VALUE_SOURCE_DEFAULT: | ||
| return "default"; | ||
| case FirebaseRemoteConfig.VALUE_SOURCE_REMOTE: | ||
| return "remote"; | ||
| case FirebaseRemoteConfig.VALUE_SOURCE_STATIC: |
There was a problem hiding this comment.
Duplicate return cases, default and static return the same thing
| @@ -56,8 +58,7 @@ public void onDetachedFromEngine(FlutterPluginBinding binding) { | |||
| cachedThreadPool, | |||
| () -> { | |||
| Map<String, Object> configProperties = getConfigProperties(remoteConfig); | |||
| Map<String, Object> configValues = new HashMap<>(); | |||
| configValues.putAll(configProperties); | |||
| Map<String, Object> configValues = new HashMap<>(configProperties); | |||
There was a problem hiding this comment.
Passing the initial values direct to the constructor. Previous putAll call after construction was unnecessary in this case.
| Task<?> methodCallTask; | ||
| FirebaseRemoteConfig remoteConfig = getRemoteConfig(call.arguments()); | ||
|
|
||
| switch (call.method) { | ||
| case "RemoteConfig#ensureInitialized": | ||
| { | ||
| methodCallTask = remoteConfig.ensureInitialized(); | ||
| methodCallTask = Tasks.whenAll(remoteConfig.ensureInitialized()); |
There was a problem hiding this comment.
THE FIX: Currently on Android the task returned by RemoteConfig.ensureInitialized() returns a FirebaseRemoteConfigInfo instance which gets sent to the Flutter standard message codec and causes a crash as this isn't a supported value that can be serialized by the codec.
As we don't at this point care about the value of the task result, I've wrapped the task in a whenAll which will just return void when completed.
| @@ -22,6 +22,7 @@ void main() { | |||
| 'welcome': 'default welcome', | |||
| 'hello': 'default hello', | |||
| }); | |||
| await remoteConfig.ensureInitialized(); | |||
There was a problem hiding this comment.
Added a call to the previously crashing method to the tests setup - this is why we haven't seen the crash on tests.
Description
Currently on Android the task returned by
RemoteConfig.ensureInitialized()returns aFirebaseRemoteConfigInfoinstance which gets sent to the Flutter standard message codec and causes a crash as this isn't a supported value that can be serialized by the codec.As we don't at this point care about the value of the task result, I've wrapped the task in a
whenAllwhich will just return void when completed.Related Issues
Fixes #5222
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).This will ensure a smooth and quick review process. Updating the
pubspec.yamland changelogs is not required.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?