better error handling for local storage operations (#44)

* better error handling for local storage operations

* lint

* fix obsolete comments

src/PersistentFlagStore.js

@@ -0,0 +1,48 @@
+import * as utils from './utils';
+
+export default function PersistentFlagStore(storage, environment, hash, ident) {
+  const store = {};
+
+  function getFlagsKey() {
+    let key = '';
+    const user = ident.getUser();
+    if (user) {
+      key = hash || utils.btoa(JSON.stringify(user));
+    }
+    return 'ld:' + environment + ':' + key;
+  }
+
+  // Returns a Promise which will be resolved with a parsed JSON value if a stored value was available,
+  // or resolved with null if there was no value or if storage was not available.
+  store.loadFlags = () =>
+    storage.get(getFlagsKey()).then(dataStr => {
+      if (dataStr === null || dataStr === undefined) {
+        return null;
+      }
+      try {
+        let data = JSON.parse(dataStr);
+        if (data) {
+          const schema = data.$schema;
+          if (schema === undefined || schema < 1) {
+            data = utils.transformValuesToVersionedValues(data);
+          } else {
+            delete data['$schema'];
+          }
+        }
+        return data;
+      } catch (ex) {
+        return store.clearFlags().then(() => null);
+      }
+    });
+
+  // Resolves with true if successful, or false if storage is unavailable. Never rejects.
+  store.saveFlags = flags => {
+    const data = utils.extend({}, flags, { $schema: 1 });
+    return storage.set(getFlagsKey(), JSON.stringify(data));
+  };
+
+  // Resolves with true if successful, or false if storage is unavailable. Never rejects.
+  store.clearFlags = () => storage.clear(getFlagsKey());
+
+  return store;
+}

src/PersistentStorage.js

@@ -0,0 +1,79 @@
+import * as messages from './messages';
+
+// The localStorageProvider is provided by the platform object. It should have the following
+// methods, each of which should return a Promise:
+// - get(key): Gets the string value, if any, for the given key
+// - set(key, value): Stores a string value for the given key
+// - remove(key): Removes the given key
+//
+// Storage is just a light wrapper of the localStorageProvider, adding error handling and
+// ensuring that we don't call it if it's unavailable. The get method will simply resolve
+// with an undefined value if there is an error or if there is no localStorageProvider.
+// None of the promises returned by Storage will ever be rejected.
+//
+// It is always possible that the underlying platform storage mechanism might fail or be
+// disabled. If so, it's likely that it will keep failing, so we will only log one warning
+// instead of repetitive warnings.
+export default function PersistentStorage(localStorageProvider, logger) {
+  const storage = {};
+  let loggedError = false;
+
+  const logError = err => {
+    if (!loggedError) {
+      loggedError = true;
+      logger.warn(messages.localStorageUnavailable(err));
+    }
+  };
+
+  storage.isEnabled = () => !!localStorageProvider;
+
+  // Resolves with a value, or undefined if storage is unavailable. Never rejects.
+  storage.get = key =>
+    new Promise(resolve => {
+      if (!localStorageProvider) {
+        resolve(undefined);
+        return;
+      }
+      localStorageProvider
+        .get(key)
+        .then(resolve)
+        .catch(err => {
+          logError(err);
+          resolve(undefined);
+        });
+    });
+
+  // Resolves with true if successful, or false if storage is unavailable. Never rejects.
+  storage.set = (key, value) =>
+    new Promise(resolve => {
+      if (!localStorageProvider) {
+        resolve(false);
+        return;
+      }
+      localStorageProvider
+        .set(key, value)
+        .then(() => resolve(true))
+        .catch(err => {
+          logError(err);
+          resolve(false);
+        });
+    });
+
+  // Resolves with true if successful, or false if storage is unavailable. Never rejects.
+  storage.clear = key =>
+    new Promise(resolve => {
+      if (!localStorageProvider) {
+        resolve(false);
+        return;
+      }
+      localStorageProvider
+        .clear(key)
+        .then(() => resolve(true))
+        .catch(err => {
+          logError(err);
+          resolve(false);
+        });
+    });
+
+  return storage;
+}

src/UserValidator.js

@@ -12,23 +12,13 @@ import * as utils from './utils';
 
 const ldUserIdKey = 'ld:$anonUserId';
 
-export default function UserValidator(localStorageProvider, logger) {
+export default function UserValidator(persistentStorage) {
   function getCachedUserId() {
-    if (localStorageProvider) {
-      return localStorageProvider.get(ldUserIdKey).catch(() => null);
-      // Not logging errors here, because if local storage fails for the get, it will presumably fail for the set,
-      // so we will end up logging an error in setCachedUserId anyway.
-    }
-    return Promise.resolve(null);
+    return persistentStorage.get(ldUserIdKey);
   }
 
   function setCachedUserId(id) {
-    if (localStorageProvider) {
-      return localStorageProvider.set(ldUserIdKey, id).catch(() => {
-        logger.warn(messages.localStorageUnavailableForUserId());
-      });
-    }
-    return Promise.resolve();
+    return persistentStorage.set(ldUserIdKey, id);
   }
 
   const ret = {};

src/__tests__/LDClient-localstorage-test.js

@@ -86,20 +86,22 @@ describe('LDClient local storage', () => {
     });
 
     it('should handle localStorage.get returning an error', async () => {
-      platform.localStorage.get = () => Promise.reject(new Error());
+      const myError = new Error('deliberate error');
+      platform.localStorage.get = () => Promise.reject(myError);
       const flags = { 'enable-foo': { value: true } };
 
       await withServer(async server => {
         server.byDefault(respondJson(flags));
         await withClient(user, { baseUrl: server.url }, async client => {
           await client.waitForInitialization();
-          expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable()]);
+          expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable(myError)]);
         });
       });
     });
 
     it('should handle localStorage.set returning an error', async () => {
-      platform.localStorage.set = () => Promise.reject(new Error());
+      const myError = new Error('deliberate error');
+      platform.localStorage.set = () => Promise.reject(myError);
       const flags = { 'enable-foo': { value: true } };
 
       await withServer(async server => {
@@ -109,7 +111,7 @@ describe('LDClient local storage', () => {
 
           await sleepAsync(0); // allow any pending async tasks to complete
 
-          expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable()]);
+          expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable(myError)]);
         });
       });
     });

src/__tests__/Store-test.js → src/__tests__/PersistentFlagStore-test.js

@@ -2,18 +2,20 @@ import * as stubPlatform from './stubPlatform';
 
 import * as messages from '../messages';
 import Identity from '../Identity';
-import Store from '../Store';
+import PersistentFlagStore from '../PersistentFlagStore';
+import PersistentStorage from '../PersistentStorage';
 import * as utils from '../utils';
 
-describe('Store', () => {
+describe('PersistentFlagStore', () => {
   const user = { key: 'user' };
   const ident = Identity(user);
   const env = 'ENVIRONMENT';
   const lsKey = 'ld:' + env + ':' + utils.btoa(JSON.stringify(user));
 
   it('stores flags', async () => {
     const platform = stubPlatform.defaults();
-    const store = Store(platform.localStorage, env, '', ident, platform.testing.logger);
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, '', ident, platform.testing.logger);
 
     const flags = { flagKey: { value: 'x' } };
 
@@ -26,7 +28,8 @@ describe('Store', () => {
 
   it('retrieves and parses flags', async () => {
     const platform = stubPlatform.defaults();
-    const store = Store(platform.localStorage, env, '', ident, platform.testing.logger);
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, '', ident, platform.testing.logger);
 
     const expected = { flagKey: { value: 'x' } };
     const stored = { $schema: 1, ...expected };
@@ -38,7 +41,8 @@ describe('Store', () => {
 
   it('converts flags from old format if schema property is missing', async () => {
     const platform = stubPlatform.defaults();
-    const store = Store(platform.localStorage, env, '', ident, platform.testing.logger);
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, '', ident, platform.testing.logger);
 
     const oldFlags = { flagKey: 'x' };
     const newFlags = { flagKey: { value: 'x', version: 0 } };
@@ -50,28 +54,31 @@ describe('Store', () => {
 
   it('returns null if storage is empty', async () => {
     const platform = stubPlatform.defaults();
-    const store = Store(platform.localStorage, env, '', ident, platform.testing.logger);
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, '', ident, platform.testing.logger);
 
     const values = await store.loadFlags();
     expect(values).toBe(null);
   });
 
   it('clears storage and returns null if value is not valid JSON', async () => {
     const platform = stubPlatform.defaults();
-    const store = Store(platform.localStorage, env, '', ident, platform.testing.logger);
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, '', ident, platform.testing.logger);
 
     platform.testing.setLocalStorageImmediately(lsKey, '{bad');
 
-    await expect(store.loadFlags()).rejects.toThrow();
+    expect(await store.loadFlags()).toBe(null);
 
     expect(platform.testing.getLocalStorageImmediately(lsKey)).toBe(undefined);
   });
 
   it('uses hash, if present, instead of user properties', async () => {
     const platform = stubPlatform.defaults();
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
     const hash = '12345';
     const keyWithHash = 'ld:' + env + ':' + hash;
-    const store = Store(platform.localStorage, env, hash, ident, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, hash, ident, platform.testing.logger);
 
     const flags = { flagKey: { value: 'x' } };
     await store.saveFlags(flags);
@@ -82,21 +89,23 @@ describe('Store', () => {
 
   it('should handle localStorage.get returning an error', async () => {
     const platform = stubPlatform.defaults();
-    const store = Store(platform.localStorage, env, '', ident, platform.testing.logger);
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, '', ident, platform.testing.logger);
     const myError = new Error('localstorage getitem error');
     jest.spyOn(platform.localStorage, 'get').mockImplementation(() => Promise.reject(myError));
 
-    await expect(store.loadFlags()).rejects.toThrow(myError);
-    expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable()]);
+    expect(await store.loadFlags()).toBe(null);
+    expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable(myError)]);
   });
 
   it('should handle localStorage.set returning an error', async () => {
     const platform = stubPlatform.defaults();
-    const store = Store(platform.localStorage, env, '', ident, platform.testing.logger);
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
+    const store = PersistentFlagStore(storage, env, '', ident, platform.testing.logger);
     const myError = new Error('localstorage setitem error');
     jest.spyOn(platform.localStorage, 'set').mockImplementation(() => Promise.reject(myError));
 
-    await expect(store.saveFlags({ foo: {} })).rejects.toThrow(myError);
-    expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable()]);
+    await store.saveFlags({ foo: {} });
+    expect(platform.testing.logger.output.warn).toEqual([messages.localStorageUnavailable(myError)]);
   });
 });

src/__tests__/diagnosticEvents-test.js

@@ -1,6 +1,6 @@
 import { baseOptionDefs } from '../configuration';
 import { DiagnosticId, DiagnosticsAccumulator, DiagnosticsManager } from '../diagnosticEvents';
-
+import PersistentStorage from '../PersistentStorage';
 import { sleepAsync } from 'launchdarkly-js-test-helpers';
 
 import * as stubPlatform from './stubPlatform';
@@ -130,12 +130,13 @@ describe('DiagnosticsManager', () => {
 
   async function withManager(extraConfig, overridePlatform, asyncCallback) {
     const platform = overridePlatform || stubPlatform.defaults();
+    const storage = PersistentStorage(platform.localStorage, platform.testing.logger);
     platform.diagnosticSdkData = sdkData;
     platform.diagnosticPlatformData = platformData;
     const config = { ...defaultConfig, ...extraConfig };
     const acc = DiagnosticsAccumulator(defaultStartTime);
     const sender = MockEventSender();
-    const m = DiagnosticsManager(platform, acc, sender, envId, config, diagnosticId);
+    const m = DiagnosticsManager(platform, storage, acc, sender, envId, config, diagnosticId);
     try {
       return await asyncCallback(m, acc, sender);
     } finally {