Make SharedPreferences return failures (flutter#441)

kurtisnelson · mehmetf · commit bf794aa7182a · 2018-04-01T09:50:58.000-07:00
On Android, SharedPreferences#apply() will silently fail if there is an
issue. Using #commit() ensures we can propogate errors up the future
chain.
Also updated the example to reflect the fact that preference writes are
not instant.
Deprecated commit() as neither platform actually implements it
correctly.
diff --git a/packages/shared_preferences/README.md b/packages/shared_preferences/README.md
@@ -3,8 +3,9 @@
 [![pub package](https://img.shields.io/pub/v/shared_preferences.svg)](https://pub.dartlang.org/packages/shared_preferences)
 
 Wraps NSUserDefaults (on iOS) and SharedPreferences (on Android), providing
-a persistent store for simple data. Data is persisted to disk automatically
-and asynchronously.
+a persistent store for simple data. Data is persisted to disk asynchronously.
+Neither platform can guarantee that writes will be persisted to disk after
+returning and this plugin must not be used for storing critical data.
 
 ## Usage
 To use this plugin, add `shared_preferences` as a [dependency in your pubspec.yaml file](https://flutter.io/platform-plugins/).
@@ -32,7 +33,7 @@ _incrementCounter() async {
   SharedPreferences prefs = await SharedPreferences.getInstance();
   int counter = (prefs.getInt('counter') ?? 0) + 1;
   print('Pressed $counter times.');
-  prefs.setInt('counter', counter);
+  await prefs.setInt('counter', counter);
 }
 ```
 
diff --git a/packages/shared_preferences/android/src/main/java/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.java b/packages/shared_preferences/android/src/main/java/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.java
@@ -5,6 +5,7 @@
 package io.flutter.plugins.sharedpreferences;
 
 import android.content.Context;
+import android.content.SharedPreferences.Editor;
 import android.util.Base64;
 import io.flutter.plugin.common.MethodCall;
 import io.flutter.plugin.common.MethodChannel;
@@ -33,7 +34,6 @@ public class SharedPreferencesPlugin implements MethodCallHandler {
   private static final String BIG_INTEGER_PREFIX = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBCaWdJbnRlZ2Vy";
 
   private final android.content.SharedPreferences preferences;
-  private final android.content.SharedPreferences.Editor editor;
 
   public static void registerWith(PluginRegistry.Registrar registrar) {
     MethodChannel channel = new MethodChannel(registrar.messenger(), CHANNEL_NAME);
@@ -43,7 +43,6 @@ public static void registerWith(PluginRegistry.Registrar registrar) {
 
   private SharedPreferencesPlugin(Context context) {
     preferences = context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
-    editor = preferences.edit();
   }
 
   private List<String> decodeList(String encodedList) throws IOException {
@@ -94,8 +93,17 @@ private Map<String, Object> getAllPrefs() throws IOException {
           // This only happens for previous usage of setStringSet. The app expects a list.
           List<String> listValue = new ArrayList<>((Set) value);
           // Let's migrate the value too while we are at it.
-          editor.remove(key);
-          editor.putString(key, LIST_IDENTIFIER + encodeList(listValue)).apply();
+          boolean success =
+              preferences
+                  .edit()
+                  .remove(key)
+                  .putString(key, LIST_IDENTIFIER + encodeList(listValue))
+                  .commit();
+          if (!success) {
+            // If we are unable to migrate the existing preferences, it means we potentially lost them.
+            // In this case, an error from getAllPrefs() is appropriate since it will alert the app during plugin initialization.
+            throw new IOException("Could not migrate set to list");
+          }
           value = listValue;
         }
         filteredPrefs.put(key, value);
@@ -107,57 +115,57 @@ private Map<String, Object> getAllPrefs() throws IOException {
   @Override
   public void onMethodCall(MethodCall call, MethodChannel.Result result) {
     String key = call.argument("key");
+    boolean status = false;
     try {
       switch (call.method) {
         case "setBool":
-          editor.putBoolean(key, (boolean) call.argument("value")).apply();
-          result.success(null);
+          status = preferences.edit().putBoolean(key, (boolean) call.argument("value")).commit();
           break;
         case "setDouble":
           float floatValue = ((Number) call.argument("value")).floatValue();
-          editor.putFloat(key, floatValue).apply();
-          result.success(null);
+          status = preferences.edit().putFloat(key, floatValue).commit();
           break;
         case "setInt":
           Number number = call.argument("value");
+          Editor editor = preferences.edit();
           if (number instanceof BigInteger) {
             BigInteger integerValue = (BigInteger) number;
             editor.putString(key, BIG_INTEGER_PREFIX + integerValue.toString(Character.MAX_RADIX));
           } else {
             editor.putLong(key, number.longValue());
           }
-          editor.apply();
-          result.success(null);
+          status = editor.commit();
           break;
         case "setString":
-          editor.putString(key, (String) call.argument("value")).apply();
-          result.success(null);
+          status = preferences.edit().putString(key, (String) call.argument("value")).commit();
           break;
         case "setStringList":
           List<String> list = call.argument("value");
-          editor.putString(key, LIST_IDENTIFIER + encodeList(list)).apply();
-          result.success(null);
+          status = preferences.edit().putString(key, LIST_IDENTIFIER + encodeList(list)).commit();
           break;
         case "commit":
-          result.success(editor.commit());
+          // We've been committing the whole time.
+          status = true;
           break;
         case "getAll":
           result.success(getAllPrefs());
-          break;
+          return;
         case "remove":
-          editor.remove(key).apply();
-          result.success(null);
+          status = preferences.edit().remove(key).commit();
           break;
         case "clear":
-          for (String keyToDelete : getAllPrefs().keySet()) {
-            editor.remove(keyToDelete);
+          Set<String> keySet = getAllPrefs().keySet();
+          Editor clearEditor = preferences.edit();
+          for (String keyToDelete : keySet) {
+            clearEditor.remove(keyToDelete);
           }
-          result.success(editor.commit());
+          status = clearEditor.commit();
           break;
         default:
           result.notImplemented();
           break;
       }
+      result.success(status);
     } catch (IOException e) {
       result.error("IOException encountered", call.method, e);
     }
diff --git a/packages/shared_preferences/example/lib/main.dart b/packages/shared_preferences/example/lib/main.dart
@@ -30,12 +30,24 @@ class SharedPreferencesDemo extends StatefulWidget {
 
 class SharedPreferencesDemoState extends State<SharedPreferencesDemo> {
   Future<SharedPreferences> _prefs = SharedPreferences.getInstance();
+  Future<int> _counter;
 
   Future<Null> _incrementCounter() async {
     final SharedPreferences prefs = await _prefs;
     final int counter = (prefs.getInt('counter') ?? 0) + 1;
+
     setState(() {
-      prefs.setInt("counter", counter);
+      _counter = prefs.setInt("counter", counter).then((bool success) {
+        return counter;
+      });
+    });
+  }
+
+  @override
+  void initState() {
+    super.initState();
+    _counter = _prefs.then((SharedPreferences prefs) {
+      return (prefs.getInt('counter') ?? 0);
     });
   }
 
@@ -46,18 +58,21 @@ class SharedPreferencesDemoState extends State<SharedPreferencesDemo> {
         title: const Text("SharedPreferences Demo"),
       ),
       body: new Center(
-          child: new FutureBuilder<SharedPreferences>(
-              future: _prefs,
-              builder: (BuildContext context,
-                  AsyncSnapshot<SharedPreferences> snapshot) {
-                if (snapshot.connectionState == ConnectionState.waiting)
-                  return const Text('Loading...');
-                final int counter = snapshot.requireData.getInt('counter') ?? 0;
-                // ignore: prefer_const_constructors
-                return new Text(
-                  'Button tapped $counter time${ counter == 1 ? '' : 's' }.\n\n'
-                      'This should persist across restarts.',
-                );
+          child: new FutureBuilder<int>(
+              future: _counter,
+              builder: (BuildContext context, AsyncSnapshot<int> snapshot) {
+                switch (snapshot.connectionState) {
+                  case ConnectionState.waiting:
+                    return const CircularProgressIndicator();
+                  default:
+                    if (snapshot.hasError)
+                      return new Text('Error: ${snapshot.error}');
+                    else
+                      return new Text(
+                        'Button tapped ${snapshot.data} time${ snapshot.data == 1 ? '' : 's' }.\n\n'
+                            'This should persist across restarts.',
+                      );
+                }
               })),
       floatingActionButton: new FloatingActionButton(
         onPressed: _incrementCounter,
diff --git a/packages/shared_preferences/ios/Classes/SharedPreferencesPlugin.m b/packages/shared_preferences/ios/Classes/SharedPreferencesPlugin.m
@@ -21,41 +21,43 @@ + (void)registerWithRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar {
       NSString *key = arguments[@"key"];
       NSNumber *value = arguments[@"value"];
       [[NSUserDefaults standardUserDefaults] setBool:value.boolValue forKey:key];
-      result(nil);
+      result(@YES);
     } else if ([method isEqualToString:@"setInt"]) {
       NSString *key = arguments[@"key"];
       NSNumber *value = arguments[@"value"];
       // int type in Dart can come to native side in a variety of forms
       // It is best to store it as is and send it back when needed.
       // Platform channel will handle the conversion.
       [[NSUserDefaults standardUserDefaults] setValue:value forKey:key];
-      result(nil);
+      result(@YES);
     } else if ([method isEqualToString:@"setDouble"]) {
       NSString *key = arguments[@"key"];
       NSNumber *value = arguments[@"value"];
       [[NSUserDefaults standardUserDefaults] setDouble:value.doubleValue forKey:key];
-      result(nil);
+      result(@YES);
     } else if ([method isEqualToString:@"setString"]) {
       NSString *key = arguments[@"key"];
       NSString *value = arguments[@"value"];
       [[NSUserDefaults standardUserDefaults] setValue:value forKey:key];
-      result(nil);
+      result(@YES);
     } else if ([method isEqualToString:@"setStringList"]) {
       NSString *key = arguments[@"key"];
       NSArray *value = arguments[@"value"];
       [[NSUserDefaults standardUserDefaults] setValue:value forKey:key];
-      result(nil);
+      result(@YES);
     } else if ([method isEqualToString:@"commit"]) {
-      result([NSNumber numberWithBool:[[NSUserDefaults standardUserDefaults] synchronize]]);
+      // synchronize is deprecated.
+      // "this method is unnecessary and shouldn't be used."
+      result(@YES);
     } else if ([method isEqualToString:@"remove"]) {
       [[NSUserDefaults standardUserDefaults] removeObjectForKey:arguments[@"key"]];
-      result(nil);
+      result(@YES);
     } else if ([method isEqualToString:@"clear"]) {
       NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];
       for (NSString *key in getAllPrefs()) {
         [defaults removeObjectForKey:key];
       }
-      result([NSNumber numberWithBool:[[NSUserDefaults standardUserDefaults] synchronize]]);
+      result(@YES);
     } else {
       result(FlutterMethodNotImplemented);
     }
diff --git a/packages/shared_preferences/lib/shared_preferences.dart b/packages/shared_preferences/lib/shared_preferences.dart
@@ -13,8 +13,7 @@ const MethodChannel _kChannel =
 /// Wraps NSUserDefaults (on iOS) and SharedPreferences (on Android), providing
 /// a persistent store for simple data.
 ///
-/// Data is persisted to disk automatically and asynchronously. Use [commit()]
-/// to be notified when a save is successful.
+/// Data is persisted to disk asynchronously.
 class SharedPreferences {
   SharedPreferences._(this._preferenceCache);
 
@@ -79,56 +78,57 @@ class SharedPreferences {
   /// Saves a boolean [value] to persistent storage in the background.
   ///
   /// If [value] is null, this is equivalent to calling [remove()] on the [key].
-  void setBool(String key, bool value) => _setValue('Bool', key, value);
+  Future<bool> setBool(String key, bool value) => _setValue('Bool', key, value);
 
   /// Saves an integer [value] to persistent storage in the background.
   ///
   /// If [value] is null, this is equivalent to calling [remove()] on the [key].
-  void setInt(String key, int value) => _setValue('Int', key, value);
+  Future<bool> setInt(String key, int value) => _setValue('Int', key, value);
 
   /// Saves a double [value] to persistent storage in the background.
   ///
   /// Android doesn't support storing doubles, so it will be stored as a float.
   ///
   /// If [value] is null, this is equivalent to calling [remove()] on the [key].
-  void setDouble(String key, double value) => _setValue('Double', key, value);
+  Future<bool> setDouble(String key, double value) =>
+      _setValue('Double', key, value);
 
   /// Saves a string [value] to persistent storage in the background.
   ///
   /// If [value] is null, this is equivalent to calling [remove()] on the [key].
-  void setString(String key, String value) => _setValue('String', key, value);
+  Future<bool> setString(String key, String value) =>
+      _setValue('String', key, value);
 
   /// Saves a list of strings [value] to persistent storage in the background.
   ///
   /// If [value] is null, this is equivalent to calling [remove()] on the [key].
-  void setStringList(String key, List<String> value) =>
+  Future<bool> setStringList(String key, List<String> value) =>
       _setValue('StringList', key, value);
 
   /// Removes an entry from persistent storage.
-  void remove(String key) => _setValue(null, key, null);
+  Future<bool> remove(String key) => _setValue(null, key, null);
 
-  void _setValue(String valueType, String key, Object value) {
-    // Set the value in the background.
+  Future<bool> _setValue(String valueType, String key, Object value) {
     final Map<String, dynamic> params = <String, dynamic>{
       'key': '$_prefix$key',
     };
     if (value == null) {
       _preferenceCache.remove(key);
-      _kChannel.invokeMethod('remove', params);
+      return _kChannel
+          .invokeMethod('remove', params)
+          .then<bool>((dynamic result) => result);
     } else {
       _preferenceCache[key] = value;
       params['value'] = value;
-      _kChannel.invokeMethod('set$valueType', params);
+      return _kChannel
+          .invokeMethod('set$valueType', params)
+          .then<bool>((dynamic result) => result);
     }
   }
 
-  /// Completes with true once saved values have been persisted to local
-  /// storage, or false if the save failed.
-  ///
-  /// It's usually sufficient to just wait for the set methods to complete which
-  /// ensure the preferences have been modified in memory. Commit is necessary
-  /// only if you need to be absolutely sure that the data is in persistent
-  /// storage before taking some other action.
+  /// Always returns true.
+  /// On iOS, synchronize is marked deprecated. On Android, we commit every set.
+  @deprecated
   Future<bool> commit() async => await _kChannel.invokeMethod('commit');
 
   /// Completes with true once the user preferences for the app has been cleared.
diff --git a/packages/shared_preferences/test/shared_preferences_test.dart b/packages/shared_preferences/test/shared_preferences_test.dart
